Add bearer token API keys and move authentication credentials into PerRPCCredentials#169
Add bearer token API keys and move authentication credentials into PerRPCCredentials#169tminusplus merged 4 commits intorelease/apikeysfrom
Conversation
| creds, err := newRPCCredential(c) | ||
| if err != nil { | ||
| return nil, nil, err | ||
| return []grpc.DialOption{}, nil |
There was a problem hiding this comment.
is this going to be confusing behavior to the user? Why did we deviate from returning an error?
There was a problem hiding this comment.
Ah that is a mistake, it should be returning the error. Will fix.
| } | ||
| } | ||
|
|
||
| if c.enableHMAC { |
There was a problem hiding this comment.
this feels premature to check-in to the code base?
There was a problem hiding this comment.
It was previously in the codebase, so I wanted to move it along to the PerRPCCredentials so this PR wasn't destructive. Then my thought was we can remove it in a future PR when desired.
|
|
||
| type Credential struct { | ||
| accessToken string // keep unexported to prevent accidental leakage of the token. | ||
| insecure bool |
There was a problem hiding this comment.
[Nit] I'd consider inverting the name of this since reading the code feels a bit like a double negative. requiresSecureTransport or something else?
There was a problem hiding this comment.
(and yes, I guess I understand you want default to be false? Maybe then we leave it as is but rename it to allowInsecureTransport)
There was a problem hiding this comment.
Will do, I initially did it to follow curl's standard arg for this and have it follow through the code but allowInsecureTransport is nicer. Do want to keep the double negative to err on the side of ensuring the default value is to enforce a secure transport.
| ID string | ||
| secret string // secret kept private to prevent accidental access. | ||
| enableHMAC bool | ||
| insecure bool |
There was a problem hiding this comment.
[Nit] rename "insecure" as the term by itself is too vague.
…rRPCCredentials (#169) * Add bearer token API keys and introduce PerRPCCredentials * Rename insecure to allowInsecureTransport and fix err return * Also rename WithInsecure to WithInsecureTransport * Add UnimplementedRequestServiceServer to connection_test.go
3 participants
What was changed
This PR achieves several things:
$prefix_$id_$secret.GetServerConnectiontests from running aurfave appto setting up acli.Contextwhich parses flags and then connects to a server viabufconn.--insecureflag, hidden to discourage use, to allow sending authentication over insecure connections. This is important for users who are using proxies like envoy which then sets up a secure connection fromlocalhost.Why?
To add support, and tests, for our beta API keys in a way which encourages secure usage by default.
Checklist
N/A
Test Plan
oauth,apikeys, andHMAC.oauthcreds.apikeycreds.Since HMAC / request signatures was punted, I did not test the functionality but instead moved it over verbatim into the
apikeycredential to keep the refactor from being destructive.