INFOPLAT-1559 Adds auth header token expiry functionality #963
Conversation
- Adds function for signing publickey + timestamp. - Adjusts `BuildAuthHeaders` to take variadic args allowing backwards compatibiilty
- go recommended way to add extend a function - added Config as 2nd arg as opposed to optional args
- minor refactor of test
| // If timestamp is negative, set it to 0. negative values cause overflow on convertsion to uint64 | ||
| // 0 timestamps will be rejected by the server as being too old | ||
| if config.timestamp < 0 { | ||
| config.timestamp = 0 |
There was a problem hiding this comment.
this sorta conflicts in spirit to the previous block where we set the timestamp to now if it's 0
should this be moved up?
There was a problem hiding this comment.
So you're saying if a negative value is passed in, we should auto correct to current time?
There are two things here:
- need to handle a default value, e.g when
configarg isnil - need to handle when
config.timestampis a negative value. should this be valid or not. as it is now, this would send a0timestamp to the auth server which would fail
If I auto-correct to current timestamp when passing in negative value, which then passes authentication, that to me is somewhat unexpected behaviour from user perspective. I would expect if pass an invalid negative timestamp, I would fail authentication one-way or another
There was a problem hiding this comment.
gotcha, thanks for elaborating
there's definitely tradeoffs with either approach, though I don't feel strongly either way. happy to keep things as-is
Co-authored-by: 4of9 <[email protected]>
…tcontractkit/chainlink-common into INFOPLAT-1559-csa-auth-token-expiry
hendoxc
had a problem deploying
to
integration
GitHub Actions
Failure
|
|
||
| // Verify the timestamp is within the last 50ms | ||
| // This verifies that default configuration is to use the current time | ||
| assert.InDelta(t, now, timestampParsed, 50, "timestamp should be within the last 50ms") |
There was a problem hiding this comment.
nit: Should it be just after or equal tonow ? or you can use mock clock github.com/jonboulle/clockwork
| if config.version == "" { | ||
| config.version = authHeaderVersion2 | ||
| } |
There was a problem hiding this comment.
Not sure how important it is, but with the current code we theoretically allow a caller to build v2 headers while passing in a different version, which will get set in the header key.
There was a problem hiding this comment.
Yes, by design, it means this function can support upgrading to a new version
| func defaultAuthHeaderConfig() *AuthHeaderConfig { | ||
| return &AuthHeaderConfig{ | ||
| version: authHeaderVersion2, | ||
| timestamp: time.Now().UnixMilli(), |
There was a problem hiding this comment.
Is it intended that we don't use config.timestamp here? Seems counter-intuitive
There was a problem hiding this comment.
Ah I think I mixed things up, nvm!
Co-authored-by: Geert G <[email protected]>
hendoxc
had a problem deploying
to
integration
GitHub Actions
Failure
What
BuildAuthHeadersV2for signing publickey + timestamp.Why
Notes
Further implementation for implementing token refresh will in follow up tickets