feat(vault): not allowing batch token revoke #4918
Conversation
| if err := v.RevokeToken(); err != nil { | ||
| log.Entry().WithError(err).Fatal("Could not revoke token") | ||
|
|
||
| // only service tokens should be revoked and not batch tokens, the below will lookup the token and depends on the token prefix hvs. for service token |
There was a problem hiding this comment.
| // only service tokens should be revoked and not batch tokens, the below will lookup the token and depends on the token prefix hvs. for service token | |
| const serviceTokenPrefix string = "hvs." |
|
|
||
| // only service tokens should be revoked and not batch tokens, the below will lookup the token and depends on the token prefix hvs. for service token | ||
| lookupPath := "auth/token/lookup-self" | ||
| secret, err := v.GetSecret("auth/token/lookup-self") |
There was a problem hiding this comment.
| secret, err := v.GetSecret("auth/token/lookup-self") | |
| secret, err := v.GetSecret(lookupPath) |
| log.Entry().Warnf("Could not lookup token at %s, not continuing to revoke", lookupPath) | ||
| } else { | ||
| if id, ok := secret.Data["id"]; ok { | ||
| if strings.HasPrefix(id.(string), "hvs.") { |
There was a problem hiding this comment.
| if strings.HasPrefix(id.(string), "hvs.") { | |
| if strings.HasPrefix(id.(string), serviceTokenPrefix) { |
There was a problem hiding this comment.
Also id.(string) will cause a panic if id is not of type string. Maybe use instead the 2 parameter version of type assertion which provides a boolean instead of panicing? (e.g. s, ok := i.(string) )
| log.Entry().WithError(err).Fatal("Could not revoke token") | ||
| } | ||
| } | ||
| log.Entry().Warnf("Could not lookup token.Data at %s, not continuing to revoke", lookupPath) |
There was a problem hiding this comment.
doesn't this Warning has to go into an else branch, otherwise it gets executed every time the RevokeToken was successful or secret could be looked up but has not the prefix?
| @@ -281,9 +281,26 @@ func (v Client) RevokeToken() error { | |||
| // MustRevokeToken same as RevokeToken but the programm is terminated with an error if this fails. | |||
| // Should be used in defer statements only. | |||
| func (v Client) MustRevokeToken() { | |||
There was a problem hiding this comment.
Must is a quite strong word so it is a bit surprising that according to the comment it only "should" do some thing under certain conditions. Maybe call the function RevokeServiceToken instead?
|
CCFenner
changed the title
|
Thank you for your contribution! This pull request is stale because it has been open 60 days with no activity. In order to keep it open, please remove stale label or add a comment within the next 10 days. If you need a Piper team member to remove the stale label make sure to add |
|
Pull request got stale and no further activity happened. It has automatically been closed. Please re-open in case you still consider it relevant. |
|
re-working on this pr to allow batch tokens |
Changes
this is alternative to #4880 and is a clean way to do it