Skip to content
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

Add Endpoint Params to AuthSchemeParams for MultiAuth Scheme Resolution #5797

Closed

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented Jan 15, 2025

Motivation and Context

  1. MultiAuth Scheme resolution requires knowledge of the Endpoint.

  2. This is because Endpoint-based Auth Scheme properties need to be set on the resolved Auth Scheme.

  3. Similar to S3/EndpointBasedAuthParams, we need to copy all EndpointParams to the AuthSchemeParams.

  4. Also these paramaters should be populated from Endpoint Params in Auth Scheme Interceptor

Modifications

  • Updated codegen Specs for AuthSchemeParams Interface and Default AuthSchemeParams class
  • Updated AuthSchemeInterceptorSpec to populate AuthSchemeParams from EndpointParams

Testing

  • Codegen test cases

Screenshots (if appropriate)

License

  • I confirm that this pull request can be released under the Apache 2 license

@joviegas joviegas requested a review from a team as a code owner January 15, 2025 19:40
@joviegas joviegas changed the title Joviegas/autheme params spec Add Endpoint Params to AuthSchemeParams for MultiAuth Scheme Resolution Jan 15, 2025
@@ -147,6 +149,25 @@ public boolean includeParamForProvider(String name) {
return true;
}

// New Multi Auth determined by "auth" triat on Service model or operation model.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: typo: trait

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -60,7 +60,7 @@ public TypeSpec poetSpec() {
.addMethod(builderMethod())
.addType(builderImplSpec());

if (authSchemeSpecUtils.useEndpointBasedAuthProvider()) {
if (authSchemeSpecUtils.useEndpointBasedAuthProvider() || authSchemeSpecUtils.hasMultiAuthSigvOrSigv4a()) {
Copy link
Contributor

@Fred1155 Fred1155 Jan 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this can't be replaced by authSchemeSpecUtils.useEndpointParamsInAuthScheme() like below? authSchemeSpecUtils.useEndpointBasedAuthProvider() returns generateEndpointBasedParams(), which is included in useEndpointParamsInAuthScheme()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@joviegas joviegas force-pushed the joviegas/autheme_params_spec branch from 9d3e545 to 3ee618d Compare January 16, 2025 22:56
if (authSchemeSpecUtils.usesSigV4()) {
builder.addStatement("$T region = executionAttributes.getAttribute($T.AWS_REGION)", Region.class,
AwsExecutionAttribute.class);
builder.addStatement("builder.region(region)");
builder.addStatement("return $T.builder()"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted this fix back to master , since these changes were not required as we are using same code as EndpointBased params

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@joviegas
Copy link
Contributor Author

Closing the PR since we do not need this .
Discussed with Matt offline to override Auth Scheme in authSchemeWithEndpointSignerProperties in ServiceResolveEndpointInterceptor

@joviegas
Copy link
Contributor Author

Closing the PR since we do not need this .
Discussed with Matt offline to override Auth Scheme in authSchemeWithEndpointSignerProperties in ServiceResolveEndpointInterceptor

@joviegas joviegas closed this Jan 24, 2025
@joviegas
Copy link
Contributor Author

joviegas commented Feb 21, 2025

Workaround to skip default checksum calculations for operations that don't require them while ensuring MD5 checksums are added for operations that do require them.

1. Skip Default Checksum Calculation for operation which donot require checksum

Choose one of these options:

Option A: Add to AWS profile configuration

[default]
request_checksum_calculation = when_required

Or

Option B: Set system property

import software.amazon.awssdk.core.SdkSystemSetting;
import software.amazon.awssdk.services.s3.S3Client;

public class DemoClass {

    public static void main(String[] args) {
    
        System.setProperty(SdkSystemSetting.AWS_REQUEST_CHECKSUM_CALCULATION.property(), "WHEN_REQUIRED");

        S3Client s3Client = S3Client.builder()
                .build();

        s3Client.listBuckets();
    }

Adding MD5 checksum for operations which require checksum calculation

Step 1: Add Interceptor class as below

mport java.io.IOException;
import java.io.UncheckedIOException;
import java.util.Optional;
import software.amazon.awssdk.core.async.AsyncRequestBody;
import software.amazon.awssdk.core.interceptor.Context;
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
import software.amazon.awssdk.core.interceptor.SdkInternalExecutionAttribute;
import software.amazon.awssdk.core.internal.util.HttpChecksumUtils;
import software.amazon.awssdk.core.sync.RequestBody;
import software.amazon.awssdk.http.Header;
import software.amazon.awssdk.http.SdkHttpRequest;
import software.amazon.awssdk.utils.Md5Utils;

public class Md5RequiredOperationInterceptor implements ExecutionInterceptor {

    @Override
    public SdkHttpRequest modifyHttpRequest(Context.ModifyHttpRequest context, ExecutionAttributes executionAttributes) {
        boolean isHttpChecksumRequired = isHttpChecksumRequired(executionAttributes);
        boolean requestAlreadyHasMd5 = context.httpRequest().firstMatchingHeader(Header.CONTENT_MD5).isPresent();

        Optional<RequestBody> syncContent = context.requestBody();
        Optional<AsyncRequestBody> asyncContent = context.asyncRequestBody();

        if (!isHttpChecksumRequired || requestAlreadyHasMd5) {
            return context.httpRequest();
        }

        if (asyncContent.isPresent()) {
            throw new IllegalArgumentException("This operation requires a content-MD5 checksum, " +
                    "but one cannot be calculated for non-blocking content.");
        }

        if (syncContent.isPresent()) {
            try {
                String payloadMd5 = Md5Utils.md5AsBase64(syncContent.get().contentStreamProvider().newStream());
                return context.httpRequest().copy(r -> r.putHeader(Header.CONTENT_MD5, payloadMd5));
            } catch (IOException e) {
                throw new UncheckedIOException(e);
            }
        }
        return context.httpRequest();
    }

    private boolean isHttpChecksumRequired(ExecutionAttributes executionAttributes) {
        return executionAttributes.getAttribute(SdkInternalExecutionAttribute.HTTP_CHECKSUM_REQUIRED) != null
                || HttpChecksumUtils.isMd5ChecksumRequired(executionAttributes);
    }
}

Step 2: Use this interceptor in the client builder as below

import software.amazon.awssdk.services.s3.S3Client;

public class DemoChecksumRollback {

    public static void main(String[] args) {

        Md5RequiredOperationInterceptor md5RequiredOperationInterceptor =
                new Md5RequiredOperationInterceptor();

        S3Client s3Client = S3Client.builder()
                .overrideConfiguration(override ->
                        override.addExecutionInterceptor(md5RequiredOperationInterceptor))
                .build();

        s3Client.listBuckets();
    }
}

3. Doing Both: skipping Checksum for operations that do not require checksum and adding MD5 checksum for operations that require checksum

import software.amazon.awssdk.services.s3.S3Client;

public class DemoChecksumRollback {
    public static void main(String[] args) {

        System.setProperty(SdkSystemSetting.AWS_REQUEST_CHECKSUM_CALCULATION.property(), "WHEN_REQUIRED");
        // Md5RequiredOperationInterceptor is same as class defined in above section
        Md5RequiredOperationInterceptor md5RequiredOperationInterceptor = 
                new Md5RequiredOperationInterceptor();

        S3Client s3Client = S3Client.builder()
                .overrideConfiguration(override ->
                        override.addExecutionInterceptor(md5RequiredOperationInterceptor))
                .build();

        s3Client.listBuckets();
    }
}

Note: The above workarounds will not prevent adding Default checksums for operations which require checksum like DeleteObjects etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants