Conversation
…idate the incoming ETAT (e2b traffic access token)
matthewlouisbrockman
requested review from
ValentaTomas,
dobrac and
jakubno
as code owners
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21f37f5335
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ValentaTomas
changed the title
| isNonEnvdTraffic := true | ||
| if vals := incomingMetadata.Get(proxygrpc.MetadataSandboxRequestPort); len(vals) > 0 { | ||
| requestPort, parseErr := strconv.ParseUint(vals[0], 10, 64) | ||
| if parseErr != nil { | ||
| logger.L().Warn( | ||
| ctx, | ||
| "invalid sandbox request port metadata for resume", | ||
| zap.Error(parseErr), | ||
| zap.String("request_port", vals[0]), | ||
| logger.WithSandboxID(sandboxID), | ||
| ) | ||
| } else { | ||
| isNonEnvdTraffic = requestPort != uint64(consts.DefaultEnvdServerPort) | ||
| } | ||
| } |
There was a problem hiding this comment.
can this be handled in some helper function?
| } | ||
|
|
||
| // Validate traffic access token for sandboxes with private ingress. | ||
| if network != nil && network.Ingress != nil && network.Ingress.AllowPublicAccess != nil && !*network.Ingress.AllowPublicAccess && isNonEnvdTraffic { |
There was a problem hiding this comment.
shouldn't you do basically the same for envdTraffic?
There was a problem hiding this comment.
yes - was considering it out of scope (we weren't doing auth) but makes sense to include it! will get that in
tests/integration/internal/tests/proxies/traffic_access_token_test.go
Outdated
Show resolved
Hide resolved
tests/integration/internal/tests/proxies/traffic_access_token_test.go
Outdated
Show resolved
Hide resolved
tests/integration/internal/tests/proxies/traffic_access_token_test.go
Outdated
Show resolved
Hide resolved
tests/integration/internal/tests/proxies/traffic_access_token_test.go
Outdated
Show resolved
Hide resolved
|
moving to draft while looking at the auth |
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…esn't need to duplicate
|
added the envd check to this as well |
| return network != nil && network.Ingress != nil && network.Ingress.AllowPublicAccess != nil && !*network.Ingress.AllowPublicAccess | ||
| } | ||
|
|
||
| func tokensMatch(providedToken string, expectedToken string) bool { |
There was a problem hiding this comment.
could you explain why we can't just compare the strings?
There was a problem hiding this comment.
was using subtle.ConstantTimeCompare to avoid doing direct string comparisong - #1909 (comment) was complaining about it
There was a problem hiding this comment.
*am using. if there's not enough variance that it's not a real concern can go back to the original string comparison
tests/integration/internal/tests/proxies/traffic_access_token_test.go
Outdated
Show resolved
Hide resolved
tests/integration/internal/tests/proxies/traffic_access_token_test.go
Outdated
Show resolved
Hide resolved
…or sandbox (just need on the python server)
4 participants
Check resuming sandbox has prior credentials over http when
allow_public_traffic:falseand envd whenprivateare set.Passes the relevant token via gRPC headers to the API which validates before allowing resuming.