INFOPLAT-1559 Adds auth header token expiry functionality #963
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jmank88
reviewed
Dec 4, 2024
jmank88
reviewed
Dec 4, 2024
jmank88
previously approved these changes
Dec 4, 2024
4of9
reviewed
Dec 4, 2024
| // 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
#967) * [loop/EnvConfig] parse sets TelemetryEmitterBatchProcessor, TelemetryEmitterExportTimeout * [beholder/client] BatchProcessor ExportTimeout option is non-zero value * [loop/EnvConfig] Use maps.Equal in tests --------- Co-authored-by: Patrick <patrick.huie@smartcontract.com>
Co-authored-by: Geert G <117188496+cll-gg@users.noreply.github.com>
Signed-off-by: Lei <lei.shi@smartcontract.com>
* define finality violation error Signed-off-by: Dmytro Haidashenko <dmytro.haidashenko@smartcontract.com> * rename finality violation Signed-off-by: Dmytro Haidashenko <dmytro.haidashenko@smartcontract.com> * Test ContainsError Signed-off-by: Dmytro Haidashenko <dmytro.haidashenko@smartcontract.com> --------- Signed-off-by: Dmytro Haidashenko <dmytro.haidashenko@smartcontract.com> Co-authored-by: Domino Valdano <domino.valdano@smartcontract.com>
* Add 'majority' aggregation method to Reduce Aggregator * (refactor): Change implementation to 'ModeQuorum' * Only fill modeQuorum for method mode
* [CAPPL-382] Normalize owner before comparing * [CAPPL-366] Add name to hash to generate workflowID
* Add pkg/pg with dialects.go & txdb.go Neither of these were in the actual pg package in chainlink repo. dialects.go came from core/store/dialects and txdb.go from core/internal/testutils/pgtest, but neither of these seem like they deserve their own package in chainlink-common--we can lump all the postgres specific common utilities under pkg/pg * Add TestTxDBDriver, NewSqlxDB, SkipShort, SkipShortDB and SkipFlakey * Add idempotency test of RegisterTxDb * Create ctx from testing context, instead of using context.Background * Only abort tx's when last connection is closed Also: convert rest of panic isn't ordinary errors * go mod tidy * Split abort channel into one per connection object All txdb connections share the same underlying connection to the postgres db. Calling NewSqlxDB() or NewConnection() with dialect=txdb doesn't create a new pg connection, it just creates a new tx with BEGIN. Closing the connection with db.Close() issues ROLLBACK. Both NewSqlxDB() and NewConneciton() choose random UUID's for their dsn string, so we shouldn't have a case where the same dsn is opened more than once. If that did happen, then these two different txdb "connections" would be sharing the same transaction which would mean closing the abort channel due to a query sent over one of them would affect the other. Hopefully that's not a problem? If it is I think our only option will be to go back to using context.Background for all queries. Before this commit, there was only one abort channel for the entire txdb driver meaning that even two entirely different connections opened with different dsn's could interfere with each other's queries. This should fix that case, which is presumably the only case we care about. Since each dsn corresponds to a different call to NewSqlxDB() and the UUID's are generated randomly, there should no longer be a conflict. Each txdb connection will have its own abort channel. * Errorf -> Fatalf on failure to register txdb driver * Add in-memory DataSource using go-duckdb * Fall back to testing txdb with in-memory backed db if CL_DATABASE_URL is not set This allows us to test most of it in CI, and all locally * Fix imports & fmt.Sprintf -> t.Log * Add concurrency test for RegisterTxDb() * Fix race condition This showed up in some of the unit tests in the linked PR in chainlink repo * Remove pg.SkipDB(), add DbUrlOrInMemory() * pkg/pg -> pkg/sqlutil/pg * NewSqlxDB -> NewTestDB, DbUrlOrInMemory -> TestURL
…onfig (#968) * wip * tests
hendoxc
force-pushed
the
INFOPLAT-1559-csa-auth-token-expiry
branch
from
111a2aa to
decbd1f
Compare
hendoxc
requested review from
a team,
nolag,
pablolagreca and
Atrax1
as code owners
|
Closing in favour of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What
BuildAuthHeadersV2for signing publickey + timestamp.Why
Notes
Further implementation for implementing token refresh will in follow up tickets