Skip to content
This repository has been archived by the owner on Jul 12, 2023. It is now read-only.

legacy signing fixes #1624

Merged
merged 4 commits into from
Jan 16, 2021
Merged

Conversation

mikehelmick
Copy link
Contributor

Fixes #1623

Proposed Changes

  • allow for tokens to be signed w/ legacy keys until DB one is present
    • can be disabled by flag
    • to be removed in v0.21
  • fix bug where a not found on DB backed key lookup returned error instead of validating w/ legacy key
  • debug logging by default in some tools - makes life easier

Release Note

Allow for legacy signing key config for tokens to be used during the upgrade to DB backed tokens.

@googlebot googlebot added the cla: yes Auto: added by CLA bot when all committers have signed a CLA. label Jan 16, 2021
@@ -95,8 +95,9 @@ func (c *Controller) validateToken(ctx context.Context, verToken string) (string
return nil, fmt.Errorf("no key corresponds to kid %q", kid)
}
tokenSigningKey = &database.TokenSigningKey{KeyVersionID: keyID}
} else {
return nil, fmt.Errorf("failed to lookup token signing key: %w", err)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the bug - it filled in legacy key ID, but then returned an error.

Copy link
Member

@sethvargo sethvargo left a comment

Choose a reason for hiding this comment

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

I’m not convinced the envvar buys us anything here? I think we should just fallback without the additional layer of indirection and remove in the next release

@@ -41,6 +42,9 @@ func main() {

ctx, done := signalcontext.OnInterrupt()

if os.Getenv("LOG_LEVEL") == "" {
Copy link
Member

Choose a reason for hiding this comment

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

It’d be better just to use the debugger logger instead of setting the env. Just don’t use logging.NewFromEnv and use the NewDebug method (I think there’s one)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this still allows the user to specify

@mikehelmick
Copy link
Contributor Author

I’m not convinced the envvar buys us anything here? I think we should just fallback without the additional layer of indirection and remove in the next release

meaning remove the flag?

@sethvargo
Copy link
Member

Yea, I think we just always fallback in this release.

@mikehelmick
Copy link
Contributor Author

Yea, I think we just always fallback in this release.

done. Always on in this release.

@google-oss-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikehelmick, sethvargo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [mikehelmick,sethvargo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@google-oss-robot google-oss-robot merged commit eb3aa09 into google:main Jan 16, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 18, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes Auto: added by CLA bot when all committers have signed a CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tokens signed w/ legacy key fail to validate if no DB keys exist`
4 participants