mirror of
https://github.com/gravitational/teleport.git
synced 2026-10-12 02:15:01 +00:00
Tiago Silva
d28831f04f
Teleport Auth server allows clients that do not present a certificate to access parts of its API. This deliberate bypass exists to support joining methods or password logins where a user or server does not have a certificate yet but has a secret that can be exchanged for a certificate.
In order to keep the system compatible and reduce the permutations required, Teleport assigned these unauthenticated connections a role called `Nop`. This role has no permissions and the only reason why it exists is so we can still build a context and checker.
This role was historically embedded in a structure called `BuiltinRole`. This struct implements the `Identity` interface and by its documentation is used by teleport services that belong to this particular cluster.
Some of the Auth server handlers are restricted to server identities and the check consists in something like:
```go
authCtx, err := s.authorizer.Authorize(ctx)
if err != nil {
return nil, trace.Wrap(err)
}
if b, ok := authCtx.Identity.(authz.BuiltinRole); !ok || !b.IsServer() {
return nil, trace.AccessDenied("only server identities can verify validated MFA challenges")
}
```
As seen above, part of the verification consists in type asserting the `authCtx.Identity` to confirm it belongs to a local server and then we check if the role belongs to an actual server.
In our codebase, we have a function called `IsLocalOrRemoteService` with the following body:
```go
// IsLocalOrRemoteService checks if the identity is either a local or remote service.
func IsLocalOrRemoteService(authContext Context) bool {
switch authContext.UnmappedIdentity.(type) {
case BuiltinRole, RemoteBuiltinRole:
return true
default:
return false
}
}
```
Given that unauthenticated users were wrapped in `BuiltinRole`, this function would return true for them. This caused the encrypted recordings service to authorize unauthenticated clients to access the upload handlers. This was the only access check applied, so the bug allowed any client to upload and replace files in the backend storage.
This change fixes that by enforcing proper RBAC checks in the encrypted recordings `CreateUpload`, `UploadPart` and `CompleteUpload` handlers. It also forbids `RemoteBuiltinRole` (remote/leaf cluster identity) from uploading files to root clusters.
This change also moves RoleNop out of the `SystemRole` type and into a new distinct type, `UnauthenticatedRole`, so the compiler prevents it from being used anywhere a real authenticated system role is expected. As a defence-in-depth measure, the middleware now rejects any TLS certificate that claims the Nop role in its Groups or SystemRoles fields.
This PR fixes a separate issue on top of the existing one where certificate rotation of encrypted sessions was available to unauthenticated users.
The protection introduced was `authCtx.AuthorizeAdminAction()`, which verifies if someone has completed the MFA challenge (if required). That was the intended idea, but given how Teleport handles unauthenticated users, they were marked with AdminActionAuthNotRequired, which bypassed MFA checks. Because of that, any client - authenticated or unauthenticated - with the ability to complete the MFA challenge (or if MFA was not required) could rotate the encryption keys for session recordings.
This doesn't significantly affect Teleport since the private keys are never exposed externally and previous keys always remain in the backend object, allowing old recordings to still be decrypted. The main concern is if the keys were rotated so many times that the backend item size exceeds the 400KB limit on DynamoDB. While this would cause the rotation to fail beforehand, it would also prevent any further rotations from succeeding.
This PR introduces proper RBAC for modifying types.KindRecordingEncryption and ensures that unauthenticated users are always assigned AdminActionAuthNotRequired.
Fixes https://github.com/gravitational/teleport-private/issues/2430
Fixes https://github.com/gravitational/teleport-private/issues/2429
Signed-off-by: Tiago Silva <[email protected]>