[api] security hardening for microsoft auth #3803
Conversation
|
Removed vultr server and associated DNS entries |
freemvmt
force-pushed
the
api-auth-security-hardening
branch
4 times, most recently
from
2d24531 to
fa078d6
Compare
freemvmt
changed the title
| @@ -112,7 +112,8 @@ assert(process.env.UNIFORM_SUBMISSION_URL); | |||
| // needed for storing original URL to redirect to in login flow | |||
| app.use( | |||
| cookieSession({ | |||
| maxAge: 24 * 60 * 60 * 100, | |||
There was a problem hiding this comment.
value was previously set at 2.4hrs but I think was supposed to be 24hrs (assuming 100 was intended as 1000), so new value is only a slight reduction, and also clearer to read
| tokenSet, | ||
| done, | ||
| ): Promise<void> => { | ||
| // TODO: use tokenSet.state to pass the redirectTo query param through the auth flow | ||
| // TODO: validate id_token sig with the public key from the jwks_uri (...v2.0/keys) |
There was a problem hiding this comment.
It's not especially clear how to do this using openid-client v5x, but they completely rewrote the package for v6 and that might offer a clearer route to fetching the keys and verifying the signature
Related task in icebox: https://trello.com/c/7BMpYwyt
| @@ -39,7 +39,8 @@ function setJWTCookie(returnTo: string, res: Response, req: Request) { | |||
| maxAge: new Date( | |||
| new Date().setFullYear(new Date().getFullYear() + 1), | |||
| ).getTime(), | |||
| sameSite: "none", | |||
| // the JWT/auth cookies should be sent only between the API server and editor | |||
| sameSite: "strict", | |||
There was a problem hiding this comment.
Please correct me if this doesn't make sense!
There was a problem hiding this comment.
I think this needs to be "none" for login to work between the Pizzas and Staging API. Worth rebasing this once #3890 is merged and testing?
There was a problem hiding this comment.
Riiight, that's the motivation for none, I hadn't thought about that case. Although this change should affect both means of authenticating, and Google seems to work on the pizza 🤔
There was a problem hiding this comment.
Will do as you suggest and see what happens
freemvmt
force-pushed
the
api-auth-security-hardening
branch
2 times, most recently
from
fd21b41 to
81aeed5
Compare
|
@freemvmt - one thing I just realised here! The changes to the API files here aren't being run when we're logging into the pizza as we're hitting the staging API for logging in. So that means in order to test this, we need to merge to I'm struggling to find the PR but I'm pretty sure that |
@DafyddLlyr that's a great point... I think I'll just take your word for it! I was testing out Aaaand perhaps now isn't the time to find out. I'll swap out for |
freemvmt
force-pushed
the
api-auth-security-hardening
branch
from
b7858f3 to
7c958a0
Compare
|
Closed in favour of perfect copy #3895 - @DafyddLlyr please approve that one instead! |
Ticket: https://trello.com/c/0gYN7cdt
Some changes to improve the security posture of the Microsoft auth flow.