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

Unable to enable ClientLogMode for request signing on individual operation. #917

Closed
jasdel opened this issue Nov 19, 2020 · 1 comment · Fixed by #955
Closed

Unable to enable ClientLogMode for request signing on individual operation. #917

jasdel opened this issue Nov 19, 2020 · 1 comment · Fixed by #955
Labels
breaking-change Issue requires a breaking change to remediate. bug This issue is a bug.

Comments

@jasdel
Copy link
Contributor

jasdel commented Nov 19, 2020

It is not currently possible to enable or disable request signing via ClientLogMode API Client Options on individual operations. This configuration can only be provided when the API Client is first initialized, and only if a custom HTTPSignerV4 is not provided. (Its OK that the log option is ignored if custom signer is used.)

Issues

There is also no way to enable LogSigning per operation call because the addHTTPSignerV4Middleware does not pass down logging level.

The default v4 signer's LogSigning is only modified when when API client is created without custom HTTPSignerV4.

func resolveHTTPSignerV4(o *Options) {
if o.HTTPSignerV4 != nil {
return
}
o.HTTPSignerV4 = v4.NewSigner(
func(s *v4.Signer) {
s.Logger = o.Logger
s.LogSigning = o.ClientLogMode.IsSigning()
s.DisableURIPathEscaping = true
},
)
}

When an Operation is invoked and the addHTTPSignerV4Middleware helper is run to add the signing middleware, the helper takes functional parameters instead of a structure. This would make it impossible to add additional parameters in the future without using a different middleware helper in the v4 package.

func addHTTPSignerV4Middleware(stack *middleware.Stack, o Options) error {
return stack.Finalize.Add(v4.NewSignHTTPRequestMiddleware(o.Credentials, o.HTTPSignerV4), middleware.After)
}

// NewSignHTTPRequestMiddleware constructs a SignHTTPRequest using the given Signer for signing requests
func NewSignHTTPRequestMiddleware(credentialsProvider aws.CredentialsProvider, signer HTTPSigner) *SignHTTPRequest {
return &SignHTTPRequest{credentialsProvider: credentialsProvider, signer: signer}
}

The HTTPSignerV4 API client generated interface does not provide functional options for additional configurations.

type HTTPSignerV4 interface {
SignHTTP(ctx context.Context, credentials aws.Credentials, r *http.Request, payloadHash string, service string, region string, signingTime time.Time) error
}

Proposal

Update v4.NewSignHTTPRequestMiddleware to take structure for options instead of individual function parameters. This will allow future options to be passed down from the operation's adding the middleware.

type SignHTTPRequestMiddlewareOptions struct {
     Credentials aws.CredentialsProvider
     Signer         HTTPSigner
     LogSigning bool
}

func NewSignHTTPRequestMiddleware(options SignHTTPRequestMiddlewareOptions) *SignHTTPRequest {
     return &SignHTTPRequest{
          credentialsProvider: options.Credentials,
          signer: options.Signer,
          logSigning: options.logSigning,
     }
}

Update the HTTPSignerV4, and HTTPSigner interfaces to to functional options for additional signing options such as LogSigning. Adding a , optFns ...func(*v4.SignerOptions) to the signature.

type HTTPSignerV4 interface {
	SignHTTP(ctx context.Context, credentials aws.Credentials, r *http.Request, payloadHash string, service string, region string, signingTime time.Time, optFns ...func(*v4.SignerOptions)) error
}
@jasdel jasdel added bug This issue is a bug. breaking-change Issue requires a breaking change to remediate. labels Nov 19, 2020
@jasdel jasdel added this to the v1.0 Release Candidate milestone Nov 19, 2020
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Issue requires a breaking change to remediate. bug This issue is a bug.
Projects
None yet
1 participant