Allow webauthn to be passed when issuing certs for web-based scp#22864
Allow webauthn to be passed when issuing certs for web-based scp#22864
Conversation
| } | ||
|
|
||
| if req.webauthn != "" { | ||
| f.issueSingleUseCert(req.webauthn, httpReq, tc) |
There was a problem hiding this comment.
Don't forget to check the error here
| } | ||
|
|
||
| if mfaReq.Required && query.Get("webauthn") == "" { | ||
| return nil, trace.BadParameter("MFA required for file transfer.") |
There was a problem hiding this comment.
If MFA is required, and MFA wasn't provided, should this be trace.AccessDenied instead?
There was a problem hiding this comment.
Yeah that makes sense. My original idea for BadParameter was "you didnt send in the parameter that was needed". I think I was in dev mode of "I need to send this parameter" but from the actual user's perspective, it makes more sense to say "You don't have access" . Thanks 👍
| cert, err := f.authClient.GenerateUserCerts(httpReq.Context(), proto.UserCertsRequest{ | ||
| PublicKey: key.MarshalSSHPublicKey(), | ||
| Username: f.ctx.GetUser(), | ||
| Expires: time.Now().Add(time.Minute).UTC(), |
There was a problem hiding this comment.
Is 1 minute enough? Should we add a little buffer to account for clock drift, or is 1m what we use elsewhere too?
There was a problem hiding this comment.
1m is used in the same way for testing connections in Discovery. I think 1m is fine as this cert is being used directly after this in the same request handler and no where else. I suppose we could make the expiry shorter if we needed, but doesn't need to be longer.
| } | ||
|
|
||
| if req.webauthn != "" { | ||
| err = f.issueSingleUseCert(req.webauthn, httpReq, tc) |
There was a problem hiding this comment.
At first it wasn't clear to me what happens to the single use cert that gets issued. Maybe a godoc on issueSingleUseCert that explains that it configures tc to use the new cert would help.
public-teleport-github-review-bot
Bot
removed the request for review
from timothyb89
avatus
force-pushed
the
michaelmyers/web_scp_with_mfa
branch
from
a839a78 to
6def1f4
Compare
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
) (#23196) * Allow webauthn to be passed when issuing certs for web-based scp * Remove extra line
) (#23195) * Allow webauthn to be passed when issuing certs for web-based scp * Remove extra line
3 participants
Currently, the web UI cannot download files from the console due to lack of MFA support. After the addition of #22528, we can pass a webauthn assertion response to generate certs. This allows the frontend to pass a param that includes their webautn assertion response and if it is present, will issue new certs. If MFA is required on the server and this key is not passed in, they will receive a "mfa required for file transfer" error, rather than a nebulous "key not found" error.
A frontend PR for this is needed and will be PRd again this branch once #22529 is merged to avoid conflicts