HADOOP-18516: [ABFS][Authentication] Support Fixed SAS Token for ABFS Authentication #6552
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
February 7, 2024 21:57
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Feb 14, 2024
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
Outdated
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/AbfsClient.java
Outdated
Show resolved
Hide resolved
...oop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java
Outdated
Show resolved
Hide resolved
| newTestFs.getFileStatus(new Path("/")); | ||
| Path testPath = new Path("/testCorrectSASToken"); | ||
| newTestFs.create(testPath).close(); | ||
| newTestFs.delete(new Path("/"), true); |
There was a problem hiding this comment.
recursive root delete is a funny one. what does abfs do here? does it delete everything? I'm curious now. (s3a fs returns false before even trying to talk to the store).
There was a problem hiding this comment.
In case of delete on a root path, ABFS list down all the children of root and delete them individually. In case a child is itself a directory, it will be deleted recursively.
There was a problem hiding this comment.
ok. I think for s3a we decided against that on the basis that it was pretty dangerous to do accidentally. nobody ever does "rm -rf /" after all. At least not more than once...
...oop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java
Outdated
Show resolved
Hide resolved
| LOG.debug("Account SAS stringToSign: " + stringToSign.replace("\n", ".")); | ||
| return computeHmac256(stringToSign); | ||
| } | ||
| } |
added 2 commits
February 19, 2024 21:01
saxenapranav
suggested changes
Feb 20, 2024
| @@ -332,6 +335,8 @@ public AbfsRestOperation setFilesystemProperties(final String properties, | |||
| final AbfsUriQueryBuilder abfsUriQueryBuilder = createDefaultUriQueryBuilder(); | |||
| abfsUriQueryBuilder.addQuery(QUERY_PARAM_RESOURCE, FILESYSTEM); | |||
|
|
|||
| appendSASTokenToQuery(ROOT_PATH, "", abfsUriQueryBuilder); | |||
There was a problem hiding this comment.
why operationName is not provided here. If the thing is that for container APIs, existing SAS mechanism can not work, we should still prevent them. We might have to add intelligence which SAS implementations are allowed for container APIs.
There was a problem hiding this comment.
Nice Catch. Operations name added for file system level operations as well.
Just FYI
The purpose of having operation name here is to determine what all permissions are needed to be set in SAS Token generated. It is up to the custom SAS Token Provider Implementation to use this information and set appropriate permissions. For Example, MockDelegationSASTokenProvider does not allow file system level operations hence it will error out for these operations with SASTokenProviderException.
Where as AccountSASGenerator will have fixed permissions irrespective of operation type.
Operation Name are added here so that if a user wants to define their own implementation, they can choose to consume this information as per their needs and logic. They can also choose to simply ignore them.
There was a problem hiding this comment.
appendSASTokenToQuery was not there before this patch. Is this added for fixed sas token only. If yes, then we would have to add if-condition to keep only fixedSasToken (if in config) applying to the container APIs, and in non-fixedSasToken case, getSASToken should not be called. Reason being, developers would already have their implementations and those might not be adaptable to the container APIs.
There was a problem hiding this comment.
As discussed, we don't want to support any container API on SAS Token Authentication. Removed these changes
| return sasTokenProvider; | ||
| } else { | ||
| // Configured Fixed SAS Token will be used to sign the requests. | ||
| return null; |
There was a problem hiding this comment.
Instead of returning null, can we create an implementation of SasTokenProvider interface, which implements getSasToken() method and return the value in the configuration FS_AZURE_SAS_FIXED_TOKEN. This would simplify code logic, remove null checks, and reduce git diffs.
| */ | ||
| @Test | ||
| public void testBothProviderFixedTokenConfigured() throws Exception { | ||
| AbfsConfiguration testAbfsConfig = getConfiguration(); |
There was a problem hiding this comment.
Since, these tests may run in parallel with test of other classes. Lets make clone of the configuration object and use it in the test.
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
Outdated
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| @@ -51,13 +50,6 @@ public interface SASTokenProvider { | |||
| String SET_PROPERTIES_OPERATION = "set-properties"; | |||
| String WRITE_OPERATION = "write"; | |||
|
|
|||
|
|
|||
| // Filesystem Level Operations | |||
There was a problem hiding this comment.
Is the purpose for removing these related to SAS issue for container API's ?
There was a problem hiding this comment.
Yes, that's right.
Theya re not there anyway on trunk. They were added in this PR only and removed later.
| @@ -32,8 +32,7 @@ | |||
| import org.apache.hadoop.fs.azurebfs.utils.ServiceSASGenerator; | |||
|
|
|||
| /** | |||
| * A mock SAS token provider implementation for testing purpose. | |||
| * Account SAS with full permission is created using storage account key. | |||
| * A mock SAS token provider implementation | |||
There was a problem hiding this comment.
. at the end of the line
|
💔 -1 overall
This message was automatically generated. |
|
:::: AGGREGATED TEST RESULT :::: HNS-OAuth[INFO] Results: HNS-SharedKey[INFO] Results: NonHNS-SharedKey[INFO] Results: AppendBlob-HNS-OAuth[INFO] Results: Time taken: 25 mins 14 secs. |
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
Show resolved
Hide resolved
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
Show resolved
Hide resolved
| * @param f source path. | ||
| * @return true if the path exists. | ||
| * @throws IOException | ||
| * @throws IOException if some issue in checking path |
...hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/FixedSASTokenProvider.java
Show resolved
Hide resolved
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
:::: AGGREGATED TEST RESULT :::: ============================================================
|
|
Thanks @steveloughran for the review. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
May 3, 2024
There was a problem hiding this comment.
+1 pending that change to abfs.md that @anmolanmol1234 spotted
steveloughran
approved these changes
May 3, 2024
steveloughran
requested changes
May 3, 2024
There was a problem hiding this comment.
I'm afraid you are very unlucky in that I have been staring at the ABFS auth code for diagnostics purposes today and so these lines of code are now very familiar.
- getPasswordString/getTrimmedPasswordString() must be used for lookup. that way you can keep the secret in a JCEKS file.
- proposed a way to avoid double wrapping exceptions
| Class<? extends SASTokenProvider> customSasTokenProviderImplementation = | ||
| getTokenProviderClass(authType, FS_AZURE_SAS_TOKEN_PROVIDER_TYPE, | ||
| null, SASTokenProvider.class); | ||
| String configuredFixedToken = this.getString(FS_AZURE_SAS_FIXED_TOKEN, null); |
There was a problem hiding this comment.
use getTrimmedPasswordString() so JECKS can be used as a store for this
There was a problem hiding this comment.
makes sense.
Taken
| LOG.trace("Using Custom SASTokenProvider implementation because it is given precedence when it is set."); | ||
| SASTokenProvider sasTokenProvider = ReflectionUtils.newInstance( | ||
| customSasTokenProviderImplementation, rawConfig); | ||
| Preconditions.checkArgument(sasTokenProvider != null, |
There was a problem hiding this comment.
better to directly raise a new TokenAccessProviderException() here so that there's no double wrapping of stack traces.
There was a problem hiding this comment.
Yes.
I realized we do have a dedicated exception type for SAS Token related issues.
Using SASTokenProviderException instead.
hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/AbfsConfiguration.java
Show resolved
Hide resolved
|
|
||
| public FixedSASTokenProvider(final String fixedSASToken) { | ||
| this.fixedSASToken = fixedSASToken; | ||
| Preconditions.checkArgument(fixedSASToken != null && !fixedSASToken.isEmpty(), |
There was a problem hiding this comment.
throw TokenAccessProviderException instead
Which change are you referring to?? |
added 2 commits
May 5, 2024 21:53
…p into HADOOP-18516-fixedSAS
Thanks for the pointers here. |
|
🎊 +1 overall
This message was automatically generated. |
steveloughran
requested changes
May 6, 2024
There was a problem hiding this comment.
+1 pending checkstyle fixes, all of which are trivial
./hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/FixedSASTokenProvider.java:25:import org.apache.hadoop.fs.azurebfs.contracts.exceptions.TokenAccessProviderException;:8: Unused import - org.apache.hadoop.fs.azurebfs.contracts.exceptions.TokenAccessProviderException. [UnusedImports]
./hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azurebfs/services/FixedSASTokenProvider.java:27:import org.apache.hadoop.util.Preconditions;:8: Unused import - org.apache.hadoop.util.Preconditions. [UnusedImports]
./hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azurebfs/ITestAzureBlobFileSystemChooseSAS.java:49: private final String TEST_PATH = "testPath";:24: Name 'TEST_PATH' must match pattern '^[a-z][a-zA-Z0-9]*$'. [MemberName]
Done. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
Hi @steveloughran @mukund-thakur |
|
🎊 +1 overall
This message was automatically generated. |
|
@anujmodi2021 merged to trunk. Can you do the branch-3.4 cherrypick, rerun the tests and then push up as a PR to merge there too? thanks |
anujmodi2021
added a commit
to anujmodi2021/hadoop
that referenced
this pull request
May 31, 2024
… Authentication (apache#6552) Contributed by Anuj Modi
4 tasks
Thanks a lot Steve. |
steveloughran
pushed a commit
that referenced
this pull request
Jun 7, 2024
… Authentication (#6552) Contributed by Anuj Modi
siegfriedweber
pushed a commit
to stackabletech/hadoop
that referenced
this pull request
Jul 11, 2024
… Authentication (apache#6552) Contributed by Anuj Modi
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of PR
Jira: https://issues.apache.org/jira/browse/HADOOP-18516
Changes ported from PR: #5148
This PR introduces a new configuration for Fixed SAS Tokens: "fs.azure.sas.fixed.token"
Using this new configuration, users can configure a fixed SAS Token in the account settings files itself. Ideally, this should be used with SAS Tokens that are scoped at a container or account level (Service or Account SAS), which can be considered to be a constant for one account or container, over multiple operations.
The other method of using a SAS Token remains valid as well, where a user provides a custom implementation of the SASTokenProvider interface, using which a SAS Token are obtained.
When an Account SAS Token is configured as the fixed SAS Token, and it is used, it is ensured that operations are within the scope of the SAS Token.
The code checks for whether the fixed token and the token provider class implementation are configured. In the case of both being set, preference is given to the custom SASTokenProvider implementation. It must be noted that if such an implementation provides a SAS Token which has a lower scope than Account SAS, some filesystem and service level operations might be out of scope and may not succeed.
For code changes:
LICENSE,LICENSE-binary,NOTICE-binaryfiles?