LakeFSFS - TokenProvider AWS (no login yet) #7604

Isan-Rivkin · 2024-03-26T18:39:43Z

Closes #7537 (without call to login, waiting on #7578).
No breaking changes. Currently not document and internal only.

tested with release hadoop-lakefs-assembly/0.2.4-RC.0

github-actions · 2024-03-26T18:45:12Z

E2E Test Results - DynamoDB Local - Local Block Adapter

Isan-Rivkin · 2024-03-27T16:22:21Z

@arielshaqed it's still a draft but please take a look at the draft im sure you will have comments.
If it's not clear then i'll come back and continue.

arielshaqed

THANKS!

Very nice. All code here is very good. But I think I am missing several parts, notably I don't understand how refresh can ever **re--**fresh the token, it will only set it once. I think we will need an additional executor / sleeping thread / something that periodically refreshes. So not sure what parts are included in this draft, basically.

AFAICT this requires sigV4 on the AWS endpoint. This is probably cool, but best validate versus requirements. In particular, us-east-1 is always special with regards to sigV4/sigV2.

arielshaqed · 2024-03-28T14:35:54Z

clients/hadoopfs/src/main/java/io/lakefs/Constants.java

+    public static final String TOKEN_AWS_CREDENTIALS_PROVIDER_ACCESS_KEY_SUFFIX = "token.aws.access.key";
+    public static final String TOKEN_AWS_CREDENTIALS_PROVIDER_SECRET_KEY_SUFFIX = "token.aws.secret.key";
+    public static final String TOKEN_AWS_CREDENTIALS_PROVIDER_SESSION_TOKEN_KEY_SUFFIX = "token.aws.session.token";
+    public static final String TOKEN_AWS_CREDENTIALS_PROVIDER_TOKEN_DURATION = "token.aws.sts.duration";


All of these require documentation somewhere, but especially a configuration option named "*_DURATION": what are the units, etc.?

fixed duration.
will add documentation before merging this as a final step in additional PR into this one, i just want to be sure about the actual configuration

arielshaqed · 2024-03-28T14:38:58Z

clients/hadoopfs/src/main/java/io/lakefs/LakeFSClient.java

+        String authProvider = FSConfiguration.get(conf, scheme, Constants.LAKEFS_AUTH_PROVIDER_KEY_SUFFIX, LakeFSClient.BASIC_AUTH);
+        // TODO(isan) here we will add support for other auth providers
+        if (authProvider != BASIC_AUTH) {
+            throw new IOException("lakeFS auth provider not implemented");


Include authProvider so that user has an idea what they tried to use and is not implemented.

arielshaqed · 2024-03-28T14:40:50Z

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

+import java.util.Map;
+import com.amazonaws.util.json.JSONObject;
+
+class GeneratePresignGetCallerIdentityRequest extends AmazonWebServiceRequest {


AmazonWebServiceRequest includes many more methods, and I don't understand how they interact with the methods here. Say for instance I call #setRequestCredentialsProvider. What happens?

THANK YOU this is unnecessary, removed it and adopted the code.

arielshaqed · 2024-03-28T14:41:21Z

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

+    Request<GeneratePresignGetCallerIdentityRequest> presignRequest(GeneratePresignGetCallerIdentityRequest r) throws IOException;
+}
+
+class GetCallerIdentityV4Presigner extends AWS4Signer implements  STSGetCallerIdentitPresigner{


nit:

Suggested change

class GetCallerIdentityV4Presigner extends AWS4Signer implements STSGetCallerIdentitPresigner{

class GetCallerIdentityV4Presigner extends AWS4Signer implements STSGetCallerIdentityPresigner{

arielshaqed · 2024-03-28T14:41:45Z

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

+    /**
+     * Seconds in a week, which is the max expiration time Sig-v4 accepts
+     */
+    private final static long MAX_EXPIRATION_TIME_IN_SECONDS = 60 * 60 * 24 * 7;


Why do we validate this? Doesn't a lower level / AWS server side validate this for us?

arielshaqed · 2024-03-28T14:45:00Z

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

+    }
+}
+
+interface STSGetCallerIdentitPresigner {


nit:

Suggested change

interface STSGetCallerIdentitPresigner {

interface STSGetCallerIdentityPresigner {

arielshaqed · 2024-03-28T14:45:26Z

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

+import java.util.Map;
+import com.amazonaws.util.json.JSONObject;
+
+class GeneratePresignGetCallerIdentityRequest extends AmazonWebServiceRequest {


Java no longer needs each class in a file named for that class?! Yippee!

It does unless the class is not declared as public 👀

arielshaqed · 2024-03-28T14:45:54Z

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

+    private void newToken() throws Exception {
+        String token = this.newPresignedGetCallerIdentityToken();
+        // call lakeFS to exchange the token for a lakeFS token
+        this.lakeFSAuthToken = "lakefs jwt 123";


yes added comment on that (will not merge this PR before #7578)

arielshaqed · 2024-03-28T15:01:52Z

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

+    }
+
+    @Override
+    public void refresh() {


I don't understand, it looks like this will never refresh! The first time it calls newToken it sets lakeFSAuthToken. And every time after that !needsNewToken, so it does nothing.

I can see very clearly see why it's confusing sorry for that.

Regardless of implementation which we can discuss - the goal of this function is to create a new token (with new TTL) if possible.

I'm trying to make a standard for calling refresh token, mimicking how AWSCredentialProvider did that.

I'm open for discussion on removing it for the initial release - e.g no refresh will be delivered.

So please document what it does: it clearly confuses at least one reader of the code.

Isan-Rivkin · 2024-04-04T10:25:28Z

@arielshaqed please re-review.
I have addressed all your comments.
This PR will require additional look before final approve it depends on missing functionality PR #7578 being merged.
Once that merged I'll use the calls in newToken().

arielshaqed

Neat!

How was this tested? Can we incorporate it into an Esti test, or into the contract test? I worry about regressing.

arielshaqed · 2024-04-07T07:11:49Z

clients/hadoopfs/pom.xml

@@ -287,7 +287,7 @@ To export to S3:
        <dependency>
            <groupId>io.lakefs</groupId>
            <artifactId>sdk</artifactId>
-            <version>1.12.0</version>
+            <version>1.14.0</version>


1.14 + contains all external principal API's.
So it will end up 1.14.x with the login API merged.

arielshaqed · 2024-04-07T07:12:59Z

clients/hadoopfs/src/main/java/io/lakefs/LakeFSClient.java

+            HttpBasicAuth basicAuth = (HttpBasicAuth) apiClient.getAuthentication(BASIC_AUTH);
+            basicAuth.setUsername(accessKey);
+            basicAuth.setPassword(secretKey);
+        }else {


nit:

Suggested change

}else {

} else {

arielshaqed · 2024-04-07T07:15:00Z

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

+    int stsExpirationInSeconds;
+    ApiClient lakeFSApi;
+
+    AWSLakeFSTokenProvider() {


This is a very odd constructor. It does nothing except change the scope. Can we get rid of it?

That's a default constructor I'm using it because the child class(es) TemporaryAWSCredentialsLakeFSTokenProvider initiates it and they also do some work before initiating it.

arielshaqed · 2024-04-07T07:32:30Z

clients/hadoopfs/src/main/java/io/lakefs/auth/AWSLakeFSTokenProvider.java

+    }
+
+    @Override
+    public void refresh() {


So please document what it does: it clearly confuses at least one reader of the code.

arielshaqed · 2024-04-07T07:35:04Z

clients/hadoopfs/src/main/java/io/lakefs/auth/GeneratePresignGetCallerIdentityRequest.java

@@ -0,0 +1,38 @@
+package io.lakefs.auth;


Nit: this is a value type. Could we use org.immutables.value?

arielshaqed · 2024-04-07T07:38:12Z

clients/hadoopfs/src/test/java/io/lakefs/auth/GetCallerIdentityV4PresignerTest.java

+        AWSSessionCredentials awsCreds = GetCallerIdentityV4PresignerTest.newMockAWSCreds();
+        GetCallerIdentityV4Presigner stsPresigner = new GetCallerIdentityV4Presigner();
+        GeneratePresignGetCallerIdentityRequest stsReq = new GeneratePresignGetCallerIdentityRequest(
+                new URI("https://sts.amazonaws.com"),


This needs to be configurable. Think AWS in China, govclouds, various regional endpoints, etc.

Not sure what you mean? this is configurable, it's just a test Im initiating the address, I could put other addresses.

arielshaqed · 2024-04-07T07:39:10Z

clients/hadoopfs/src/test/java/io/lakefs/auth/GetCallerIdentityV4PresignerTest.java

+        GeneratePresignGetCallerIdentityRequest stsReq = new GeneratePresignGetCallerIdentityRequest(
+                new URI("https://sts.amazonaws.com"),
+                awsCreds,
+                new HashMap<String,String>(){{


Can we use e.g. ImmutableMap?

Dependency that exists only in tests we discussed and decided not to introduce into lakeFSFS.

arielshaqed · 2024-04-07T07:39:54Z

clients/hadoopfs/src/test/java/io/lakefs/auth/GetCallerIdentityV4PresignerTest.java

+                +"Action=GetCallerIdentity&"
+                + "X-Amz-Algorithm=AWS4-HMAC-SHA256&"+


Please position "+" signs consistently.

arielshaqed · 2024-04-07T07:42:15Z

clients/hadoopfs/src/test/java/io/lakefs/auth/GetCallerIdentityV4PresignerTest.java

+                "Version=2011-06-15&"+
+                "X-Amz-SignedHeaders=host%3Bx-lakefs-server-id&"+
+                "X-Amz-Security-Token=[^&]+&"+
+                "X-Amz-Credential="+ awsCreds.getAWSAccessKeyId()+"%2F\\d{8}%2Fus-east-1%2Fsts%2Faws4_request&"+


Ideally you would extract the credential to a group using (\w+), match, and then test equality to the AWS access key ID. The current code might behave weirdly if the access key ID is somehow invalid and contains strange characters. We can work around that, or we can make an effort to explain why it cannot happen, or we can make the effort to quote the access key ID -- but it will still be simplest simply not to use a regexp to do that.

I know it's just a test. OTOH, "code returned a bad access key" would be a really good error message :-)

updated with specific check for that param and a message

arielshaqed · 2024-04-07T07:44:26Z

clients/hadoopfs/src/test/java/io/lakefs/auth/GetCallerIdentityV4PresignerTest.java

+
+        Pattern compiledPattern = Pattern.compile(pattern);
+        Matcher matcher = compiledPattern.matcher(url.toString());
+        Assert.assertEquals(String.format("URL %s does not match \npattern: %s",url, pattern), true, matcher.matches());


This unit test verifies that the code returns all query parameters in a particular order. But I do not think that this should be part of the requirement for what we are testing.

So I would really prefer to parse the HRL, and then verify that the fields are correct.

updated the test to check and compare each parameter one by one as suggested, thanks!

Isan-Rivkin · 2024-04-07T09:18:23Z

Thanks @arielshaqed addressed all your comments.
Merging this PR and waiting for #7578 to be merged.
Once it's merged I'll create additional PR with call to that API in the LakeFSClient and documentation accordingly.

initial v

ce61e6d

Isan-Rivkin self-assigned this Mar 26, 2024

Isan-Rivkin marked this pull request as draft March 26, 2024 18:39

works

ea2217e

Isan-Rivkin requested a review from arielshaqed March 27, 2024 16:21

arielshaqed reviewed Mar 28, 2024

View reviewed changes

Isan-Rivkin added 2 commits April 4, 2024 13:19

works

dc48b58

Merge branch 'master' into feature/m1-lakefsfs-iam

342126e

Isan-Rivkin requested a review from arielshaqed April 4, 2024 10:22

arielshaqed approved these changes Apr 7, 2024

View reviewed changes

update review comments

46c6306

Isan-Rivkin marked this pull request as ready for review April 7, 2024 09:16

disable using auth provider not basic auth

4b6388c

Isan-Rivkin added the include-changelog PR description should be included in next release changelog label Apr 7, 2024

Isan-Rivkin changed the title ~~LakeFSFS - TokenProvider AWS~~ LakeFSFS - TokenProvider AWS (no login yet) Apr 7, 2024

Isan-Rivkin added 4 commits April 10, 2024 10:37

Make Hadoop 2/3 compatible with Java SDK for IAM login (#7634)

280599a

minor comment fix

79ff184

Merge branch 'master' into feature/m1-lakefsfs-iam

05f2f28

bump lakeFS

ffc7128

Isan-Rivkin temporarily deployed to Treeverse signing April 10, 2024 08:02 — with GitHub Actions Inactive

Isan-Rivkin merged commit 99a60cd into master Apr 10, 2024
36 checks passed

Isan-Rivkin deleted the feature/m1-lakefsfs-iam branch April 10, 2024 09:16

emulatorchen pushed a commit to emulatorchen/lakeFS that referenced this pull request May 27, 2024

LakeFSFS - TokenProvider AWS (no login yet) (treeverse#7604)

c9f39cd

	class GetCallerIdentityV4Presigner extends AWS4Signer implements STSGetCallerIdentitPresigner{
	class GetCallerIdentityV4Presigner extends AWS4Signer implements STSGetCallerIdentityPresigner{

	interface STSGetCallerIdentitPresigner {
	interface STSGetCallerIdentityPresigner {

		+"Action=GetCallerIdentity&"
		+ "X-Amz-Algorithm=AWS4-HMAC-SHA256&"+

LakeFSFS - TokenProvider AWS (no login yet) #7604

LakeFSFS - TokenProvider AWS (no login yet) #7604

Conversation

Isan-Rivkin commented Mar 26, 2024 • edited Loading

github-actions bot commented Mar 26, 2024 • edited Loading

E2E Test Results - DynamoDB Local - Local Block Adapter

Isan-Rivkin commented Mar 27, 2024

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isan-Rivkin commented Apr 4, 2024

arielshaqed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Isan-Rivkin commented Apr 7, 2024

Isan-Rivkin commented Mar 26, 2024 •

edited

Loading

github-actions bot commented Mar 26, 2024 •

edited

Loading