Add authentication logic for API key #1193
Conversation
pipecd-bot
left a comment
pipecd-bot
left a comment
There was a problem hiding this comment.
Some issues were detected while linting go source files in your changes.
pkg/rpc/rpcauth/interceptor_test.go
Outdated
| key *model.APIKey | ||
| } | ||
|
|
||
| func (v testAPIKeyVerifier) Verify(ctx context.Context, key string) (*model.APIKey, error) { |
There was a problem hiding this comment.
ctx is unused in Verify
|
|
||
| const ( | ||
| apiKeyLength = 50 | ||
| apiKeyLength = 32 |
There was a problem hiding this comment.
does this change due to some change in requirement? 👀
There was a problem hiding this comment.
To be honest, I'm wondering about the APIKey's length.
Currently, the returned APIKey contains 2 parts like this "UUID-PART.RANDOM-PART".
- The first part is a UUID string used as the key's ID with a length of 32 characters.
- The second part is a random string used as the key's password.
If the random part is 50 characters then the whole APIKey looks pretty long.
So I reduced the length of that part to 32 and the entire length of the APIKey is 64 characters.
(Not sure, but maybe we also should encrypt the APIKey (e.g. base64) to get a better look 🤔 )
There was a problem hiding this comment.
(Not sure, but maybe we also should encrypt the APIKey (e.g. base64) to get a better look 🤔 )
I think it would be better if the key has a specific length in all cases, will reduce other issues around miss/conflict setting. What do you think? 🤔
There was a problem hiding this comment.
You are right. Nice catch!
There was a problem hiding this comment.
let's address it by another PR 😄
|
Code coverage for golang is
|
|
/approve |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
|
Code coverage for golang is
|
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #1192
Does this PR introduce a user-facing change?: