-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce aws sigv4a request signer #303
Introduce aws sigv4a request signer #303
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the changes! I have left some comments.
...in/scala/org/opensearch/flint/core/auth/ResourceBasedAWSRequestSigningApacheInterceptor.java
Outdated
Show resolved
Hide resolved
...in/scala/org/opensearch/flint/core/auth/ResourceBasedAWSRequestSigningApacheInterceptor.java
Show resolved
Hide resolved
flint-core/src/main/scala/org/opensearch/flint/core/storage/FlintOpenSearchClient.java
Outdated
Show resolved
Hide resolved
.../src/main/scala/org/opensearch/flint/core/auth/AWSRequestSigV4ASigningApacheInterceptor.java
Show resolved
Hide resolved
b8a70b7
to
2c73f2e
Compare
2c73f2e
to
b4aec55
Compare
Signed-off-by: Louis Chu <[email protected]>
b4aec55
to
c142882
Compare
.../src/main/scala/org/opensearch/flint/core/auth/AWSRequestSigV4ASigningApacheInterceptor.java
Show resolved
Hide resolved
this.primaryInterceptor = new AWSRequestSigningApacheInterceptor(service, signer, primaryCredentialsProvider); | ||
this.metadataAccessInterceptor = primaryCredentialsProvider.equals(metadataAccessCredentialsProvider) | ||
? this.primaryInterceptor | ||
: new AWSRequestSigV4ASigningApacheInterceptor(service, region, AwsCrtV4aSigner.builder().build(), metadataAccessCredentialsProvider); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a builder for AwsCrtV4asigner? Waht are the parameters available? Are we good with defaults. I see
plain constructor for AWS4Signer signer = new AWS4Signer();
Also when should we use AwsCrtV4asigner insread of AWS4Signer. what are the downsides of making AwsCrtV4asigner default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is there a builder for AwsCrtV4asigner? Waht are the parameters available? Are we good with defaults. I see plain constructor for
AWS4Signer signer = new AWS4Signer();
Also when should we use AwsCrtV4asigner insread of AWS4Signer. what are the downsides of making AwsCrtV4asigner default.
AwsCrtV4aSigner is an interface that employs the builder pattern to instantiate. However, the AWS4Signer class has its own getter and setter.
@SdkPublicApi
@Immutable
@ThreadSafe
public interface AwsCrtV4aSigner extends Signer, Presigner {
/**
* Create a default Aws4aSigner.
*/
static AwsCrtV4aSigner create() {
return DefaultAwsCrtV4aSigner.create();
}
static Builder builder() {
return DefaultAwsCrtV4aSigner.builder();
}
interface Builder {
/**
* The region scope that this signer will default to if not provided explicitly when the signer is invoked.
*
* @param defaultRegionScope The default region scope.
* @return This builder for method chaining.
*/
Builder defaultRegionScope(RegionScope defaultRegionScope);
AwsCrtV4aSigner build();
}
}
The main difference is that AWS4Signer utilizes AWS4-HMAC-SHA256, whereas AWSCrtV4asigner uses AWS4-ECDSA-P256-SHA256:
- Algorithm: The primary difference is the algorithm used for signing requests: HMAC with SHA-256 in the former, and ECDSA with P-256 and SHA-256 in the latter.
- Security: Both are considered secure, but ECDSA might offer better performance with equivalent levels of security due to the efficiency of elliptic curve cryptography.
- Implementation: AWS4Signer is from aws-java-sdk V1, whereas AwsCrtV4asigner is from V2. They are incompatible with one another.
For the long run, we could use AwsCrtV4asigner as the default. However, in this PR, I would like to confine the blast radius to only metadata access using a specified credential provider.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long term plan is replace signer with signer4a?, and deprecated AWSRequestSigningApacheInterceptor? In case the major difference of singer vs singer 4a is With AWS Signature Version 4A, the signature does not include Region-specific information and is calculated using the AWS4-ECDSA-P256-SHA256 algorithm.
I think we should use one Interceptor instead of two.
if it is correct, could u create a issue to track future maintain works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
long term plan is replace signer with signer4a?, and deprecated AWSRequestSigningApacheInterceptor? In case the major difference of singer vs singer 4a is With AWS Signature Version 4A, the signature does not include Region-specific information and is calculated using the AWS4-ECDSA-P256-SHA256 algorithm. I think we should use one Interceptor instead of two.
if it is correct, could u create a issue to track future maintain works.
static boolean skipHeader(final Header header) { | ||
return ("content-length".equalsIgnoreCase(header.getName()) | ||
&& "0".equals(header.getValue())) // Strip Content-Length: 0 | ||
|| "host".equalsIgnoreCase(header.getName()); // Host comes from endpoint | ||
&& "0".equals(header.getValue())) // Strip Content-Length: 0 | ||
|| "host".equalsIgnoreCase(header.getName()) // Host comes from endpoint | ||
|| "connection".equalsIgnoreCase(header.getName()); // Skip setting Connection manually |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: better to check if header is against a set of headers
Signed-off-by: Louis Chu <[email protected]>
Signed-off-by: Louis Chu <[email protected]>
f9e9fb8
to
c1fc8fa
Compare
Signed-off-by: Louis Chu <[email protected]>
c1fc8fa
to
03f7210
Compare
Signed-off-by: Louis Chu <[email protected]>
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/opensearch-spark/backport-0.3 0.3
# Navigate to the new working tree
pushd ../.worktrees/opensearch-spark/backport-0.3
# Create a new branch
git switch --create backport/backport-303-to-0.3
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 c877e09d0cbc45569b7e963382d93904a5801e9e
# Push it to GitHub
git push --set-upstream origin backport/backport-303-to-0.3
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/opensearch-spark/backport-0.3 Then, create a pull request where the |
* Introduce aws sigv4a request signer Signed-off-by: Louis Chu <[email protected]> * Use default provider when metadata provider is unavaliable Signed-off-by: Louis Chu <[email protected]> * Move shutdown logic to application end Signed-off-by: Louis Chu <[email protected]> * sbt fmt Signed-off-by: Louis Chu <[email protected]> --------- Signed-off-by: Louis Chu <[email protected]> (cherry picked from commit c877e09) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Introduce aws sigv4a request signer * Use default provider when metadata provider is unavaliable * Move shutdown logic to application end * sbt fmt --------- (cherry picked from commit c877e09) Signed-off-by: Louis Chu <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
AWSRequestSigV4ASigningApacheInterceptor
to sign metadata access with sigV4aResourceBasedAWSRequestSigningApacheInterceptor
interfaceCurrent signer:
New sigV4a signer
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.