Return a 403 for a bad SSCT instead of 500 #16112
Conversation
vault/request_handling.go
Outdated
| if err != nil { | ||
| return nil, fmt.Errorf("server side consistent token check failed: %w", err) | ||
| return logical.ErrorResponse(fmt.Sprintf("bad token: %s", err.Error())), logical.ErrPermissionDenied |
There was a problem hiding this comment.
Not sure it's a good idea to put the actual error message into the response, helpful though it would be.
There was a problem hiding this comment.
Sure, I can log it instead.
There was a problem hiding this comment.
I'm not sure we should log it either. Since this can be done without authentication, it could allow a bad actor to fill up log devices.
There was a problem hiding this comment.
@ncabatoff we already have logs being generated as part of the SSCT checks if those checks fail. I'm not too sure what to do here, since on one hand we don't want to leak data about the token, but on the other hand debugging failing requests will be much more difficult if we don't have the information in the logs that point to exactly what is failing (lagging WALs, wrong key, etc.). What do you suggest we do?
There was a problem hiding this comment.
All I can say is that, in general, Vault requests, especially unauthenticated ones, should not result in logging. We sometimes make exceptions, particularly when we discover something surprising/concerning about the state of vault. In this case the problem can be purely user-side though, so I think the risk of harm is more important than the help for debugging.
There was a problem hiding this comment.
Ok, I'll remove the logging.
Closes #16102