feat: secure and cross-domain subdomain-based proxying#4136
Conversation
|
Using the secret as the key works well 👍. Just make sure to comment the hash should be treated as a secret. |
kylecarbs
left a comment
There was a problem hiding this comment.
I did a thorough review, and I'm impressed with what you came up with. This implementation is very clean. Bravo! Just a few questions before merge.
| keyID, _, hashedSecret, data := generateAPIKey(t, db) | ||
| insertAPIKey(t, db, keyID, hashedSecret) | ||
|
|
||
| encrypted, err := encryptAPIKey(data) |
There was a problem hiding this comment.
What are your thoughts on moving encryptAPIKey and decryptAPIKey into a new package like appwildcard or something and moving associated helper functions there? I like all the code, but I'm worried that someone exploring the coderd package may not understand when or why we need to encrypt/decrypt keys.
There was a problem hiding this comment.
A problem we had in v1 was that the devurls code was split across many different places. I'd like to avoid that if possible in v2 by keeping all of the code in one place.
I've added a comment above each of these functions, does that solve the problem?
There was a problem hiding this comment.
Fair enough, that code wasn't consolidated well. Seems good to me!
Why not do this using Playwright which supports multiple browsers? |
jsjoeio
left a comment
There was a problem hiding this comment.
From a FE view, all looks good 👍🏼
ExtractAPIKeyand turn the parameters into a struct./api/v2/applications/auth-redirect?redirect_uri=...redirectquery parameter set) if the user is not logged in?redirect_uri=xwith?coder_application_api_key_...=$encrypted_key?coder_application_api_keyget intercepted and not proxied.apps.coder.com/api/v2/authcheckbecause I wanted to use it in a test./api/v2/users/:userid/authorizationin favor of the aboveThe new auth flow looks like this from a network perspective:
TODO: