Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

INFOPLAT-1562 dynamic expiring auth headers #974

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

hendoxc
Copy link

@hendoxc hendoxc commented Dec 13, 2024

INFOPLAT-1560

What

Allows usage of dynamic auth headers by implementing grpc.PerRPCCredentials & setting as a DialOption on the otel client

Why

Makes tokens expire, in the case that one is leaked or intercepted.

  • someone can indeed steal a token, update the timestamp portion of the token, then try send data to the gateway endpoint, however since pubkey bytes + timestamp bytes are what's being signed, the signature part of the token is invalid, and will be rejected by the gateway
  • on the gateway, version 2 tokens have their timestamp part checked time.Now > timestamp > time.Now - serverTTL

Notes

Current users of the client can still configure static headers using the AuthHeaders field of beholder client Config. To enable dynamic headers they instead should configure AuthHeaderProvider

pkg/beholder/auth.go Outdated Show resolved Hide resolved
pkg/beholder/auth.go Outdated Show resolved Hide resolved
pkg/beholder/auth.go Outdated Show resolved Hide resolved
pkg/beholder/auth.go Show resolved Hide resolved
hendoxc and others added 6 commits December 17, 2024 11:14
- Adds function for signing publickey + timestamp.
- Adjusts `BuildAuthHeaders` to take variadic args allowing backwards compatibiilty

INFOPLAT-1559 Fixes some lint issues

INFOPLAT-1559 Handles edge case of negative timestamps

INFOPLAT-1559 Switches to exposing new function

- go recommended way to add extend a function
- added Config as 2nd arg as opposed to optional args

INFOPLAT-1559 Adjusts `authHeaderVersion2`

- minor refactor of test

INFOPLAT-1559 Tightens time range for test

Update pkg/beholder/auth.go

Co-authored-by: 4of9 <[email protected]>
- allows for dynamic headers auth tokens to be used in GRPC request headers
- auto refreshes the token on interval
…c `AuthHeaders`

Need this for migration of existing usage - current users of beholder can still use never verions while using static headers, but can make the switch across to setting `AuthHeaderProvider`
bit more clear conceptually
protects against concurrent calls of `refresh`

INFOPLAT-1560 Makes `refresh` thread safe
@hendoxc hendoxc marked this pull request as ready for review December 17, 2024 20:21
@hendoxc hendoxc requested a review from a team as a code owner December 17, 2024 20:21
@hendoxc hendoxc requested review from 4of9 and cll-gg December 17, 2024 20:25
pkg/beholder/auth.go Outdated Show resolved Hide resolved
- uses method for construction
4of9
4of9 previously approved these changes Dec 19, 2024
otlpmetricgrpc.WithHeaders(config.AuthHeaders),
}
if config.AuthHeaderProvider != nil {
opts = append(opts, otlpmetricgrpc.WithDialOption(grpc.WithPerRPCCredentials(config.AuthHeaderProvider.Credentials())))
Copy link
Contributor

@pkcll pkcll Dec 20, 2024

Choose a reason for hiding this comment

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

It seems there should be a relationship between otlpmetricgrpc.WithTLSCredentials (line 355, set based on config.InsecureConnection option) and
config.AuthHeaderProvider.Credentials().RequireTransportSecurity

If TLS is enabled AuthHeaderProvider.Credentials().RequireTransportSecurity()should always return true.
Im wondering whats the good way to handle that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Lets test what happens when RequireTransportSecurity and InsecureConnection mismatch.

otlploggrpc.WithHeaders(cfg.AuthHeaders),
}
if cfg.AuthHeaderProvider != nil {
opts = append(opts, otlploggrpc.WithDialOption(grpc.WithPerRPCCredentials(cfg.AuthHeaderProvider.Credentials())))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

pkcll
pkcll previously approved these changes Dec 20, 2024
if Config.InsecureConnection is `false` we should ensure that we set our `PerRPCCredentials` implementation to return true for `RequireTransportSecurity`

While `RequireTransportSecurity` can be set on AuthHeaderProvider initiailzation,  its not required, this can lead to a mismatch in configurations, this adds a guard to prevent this from happening
@hendoxc hendoxc dismissed stale reviews from pkcll and 4of9 via d895532 December 20, 2024 19:10
pkcll
pkcll previously approved these changes Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants