-
Notifications
You must be signed in to change notification settings - Fork 7
Endpoints auth #328
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
Endpoints auth #328
Conversation
4aaa535
to
c885c23
Compare
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.
LGTM but there appears to be some RBS failure - FYI.
class ApiKeySigner | ||
def initialize(options = {}) | ||
@name = options[:name] | ||
@in = options[:in] | ||
@scheme = options[:scheme] | ||
end |
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.
Are these intended to be public interfaces - if so, we should include more documentation about init options? Here, other signers and auth scheme.
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.
I intended them to be API private for now. I'm also not sure yet so I'm going to wait on documentation.
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.
Looks good overall to me. There's a few failing RBS tests.
end | ||
|
||
def resolve_without_endpoint_auth(config, auth_parameters) | ||
auth_options = config.auth_resolver.resolve(auth_parameters) |
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.
If I'm understanding this correctly, endpoint auth completely overrides the modeled auths?
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.
Yeah. I think we should do it that way. It's less complex and I think endpoint auth is guaranteed to work.
|
||
properties[key] = value | ||
end | ||
normalized_endpoint_schemes << { scheme_id: normalized_scheme_id, signer_properties: properties } |
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.
I'm assuming we'll need to make changes to sigv4 auth in V4 as well following these changes. Have you tested with V4?
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.
Yeah, I tested with v4 and I opened a separate PR.
Resolves auth using an auth module instead of entirely in the resolve auth plugin. This is necessary so that presigners and other auth related utilities can resolve auth. The auth is now once again an auth scheme and has an attached signer and identity provider, so that signing can be done.
Also implements auth scheme preference config.