Skip to content

Commit

Permalink
support split SSH/TLS keys in SSO login endpoints (gravitational#45876)
Browse files Browse the repository at this point in the history 
* proto

* api/types

* split ssh/tls keys for sso login

* fix sso testers

* fix typo in comment

Co-authored-by: Bartosz Leper <bartosz.leper@goteleport.com>

* improve test coverage of github auth APIs

* test proxy login APIs

* lint fix

---------

Co-authored-by: Bartosz Leper <bartosz.leper@goteleport.com>
  • Loading branch information
@nklaassen @bl-nero
nklaassen and bl-nero authored Sep 4, 2024
1 parent 1c9d9ad commit acc08a3
Show file tree
Hide file tree
Showing 20 changed files with 3,401 additions and 2,110 deletions.
61 changes: 55 additions & 6 deletions api/proto/teleport/legacy/types/types.proto
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4548,9 +4548,10 @@ message OIDCAuthRequest {
// Teleport Proxy after the oidc login attempt in the browser.
string RedirectURL = 6 [(gogoproto.jsontag) = "redirect_url"];

// PublicKey is an optional public key, users want these
// keys to be signed by auth servers user CA in case
// of successful auth
// PublicKey is an optional public key, users want these keys to be signed by
// auth servers user CA in case of successful auth.
//
// Soon to be deprecated after references are removed from teleport.e.
bytes PublicKey = 7 [(gogoproto.jsontag) = "public_key"];

// CertTTL is the TTL of the certificate user wants to get
Expand Down Expand Up @@ -4589,6 +4590,8 @@ message OIDCAuthRequest {
string ProxyAddress = 16 [(gogoproto.jsontag) = "proxy_address,omitempty"];

// attestation_statement is an attestation statement for the given public key.
//
// Soon to be deprecated after references are removed from teleport.e.
teleport.attestation.v1.AttestationStatement attestation_statement = 17 [(gogoproto.jsontag) = "attestation_statement,omitempty"];

// ClientLoginIP specifies IP address of the client for login, it will be written to the user's certificates.
Expand All @@ -4597,6 +4600,17 @@ message OIDCAuthRequest {
// ClientUserAgent is the user agent of the Web browser, used for issuing a
// DeviceWebToken.
string ClientUserAgent = 19 [(gogoproto.jsontag) = "client_user_agent,omitempty"];

// SshPublicKey is an optional public key to use as the subject of an issued
// SSH cert in case of successful auth.
bytes ssh_public_key = 20 [(gogoproto.jsontag) = "ssh_pub_key,omitempty"];
// TlsPublicKey is an optional public key to use as the subject of an issued
// TLS cert in case of successful auth.
bytes tls_public_key = 21 [(gogoproto.jsontag) = "tls_pub_key,omitempty"];
// SshAttestationStatement is an attestation statement for the given SSH public key.
teleport.attestation.v1.AttestationStatement ssh_attestation_statement = 22 [(gogoproto.jsontag) = "ssh_attestation_statement,omitempty"];
// TlsAttestationStatement is an attestation statement for the given TLS public key.
teleport.attestation.v1.AttestationStatement tls_attestation_statement = 23 [(gogoproto.jsontag) = "tls_attestation_statement,omitempty"];
}

// SAMLConnectorV2 represents a SAML connector.
Expand Down Expand Up @@ -4700,6 +4714,8 @@ message SAMLAuthRequest {
// PublicKey is an optional public key, users want these
// keys to be signed by auth servers user CA in case
// of successful auth.
//
// Soon to be deprecated after references are removed from teleport.e.
bytes PublicKey = 6 [(gogoproto.jsontag) = "public_key"];

// CertTTL is the TTL of the certificate user wants to get.
Expand Down Expand Up @@ -4735,6 +4751,8 @@ message SAMLAuthRequest {
SAMLConnectorSpecV2 ConnectorSpec = 15 [(gogoproto.jsontag) = "connector_spec,omitempty"];

// attestation_statement is an attestation statement for the given public key.
//
// Soon to be deprecated after references are removed from teleport.e.
teleport.attestation.v1.AttestationStatement attestation_statement = 16 [(gogoproto.jsontag) = "attestation_statement,omitempty"];

// ClientLoginIP specifies IP address of the client for login, it will be written to the user's certificates.
Expand All @@ -4743,6 +4761,17 @@ message SAMLAuthRequest {
// ClientUserAgent is the user agent of the Web browser, used for issuing a
// DeviceWebToken.
string ClientUserAgent = 18 [(gogoproto.jsontag) = "client_user_agent,omitempty"];

// SshPublicKey is an optional public key to use as the subject of an issued
// SSH cert in case of successful auth.
bytes ssh_public_key = 19 [(gogoproto.jsontag) = "ssh_pub_key,omitempty"];
// TlsPublicKey is an optional public key to use as the subject of an issued
// TLS cert in case of successful auth.
bytes tls_public_key = 20 [(gogoproto.jsontag) = "tls_pub_key,omitempty"];
// SshAttestationStatement is an attestation statement for the given SSH public key.
teleport.attestation.v1.AttestationStatement ssh_attestation_statement = 21 [(gogoproto.jsontag) = "ssh_attestation_statement,omitempty"];
// TlsAttestationStatement is an attestation statement for the given TLS public key.
teleport.attestation.v1.AttestationStatement tls_attestation_statement = 22 [(gogoproto.jsontag) = "tls_attestation_statement,omitempty"];
}

// AttributeMapping maps a SAML attribute statement to teleport roles.
Expand Down Expand Up @@ -4835,7 +4864,12 @@ message GithubAuthRequest {
// CSRFToken is used to protect against CSRF attacks.
string CSRFToken = 4 [(gogoproto.jsontag) = "csrf_token"];
// PublicKey is an optional public key to sign in case of successful auth.
bytes PublicKey = 5 [(gogoproto.jsontag) = "public_key"];
//
// Deprecated: prefer SshPublicKey and/or TlsPublicKey.
bytes PublicKey = 5 [
(gogoproto.jsontag) = "public_key",
deprecated = true
];
// CertTTL is TTL of the cert that's generated in case of successful auth.
int64 CertTTL = 6 [
(gogoproto.jsontag) = "cert_ttl",
Expand Down Expand Up @@ -4865,13 +4899,28 @@ message GithubAuthRequest {
bool SSOTestFlow = 14 [(gogoproto.jsontag) = "sso_test_flow"];
// ConnectorSpec is embedded connector spec for use in test flow.
GithubConnectorSpecV3 ConnectorSpec = 15 [(gogoproto.jsontag) = "connector_spec,omitempty"];
// attestation_statement is an attestation statement for the given public key.
teleport.attestation.v1.AttestationStatement attestation_statement = 16 [(gogoproto.jsontag) = "attestation_statement,omitempty"];
// AttestationStatement is an attestation statement for the given public key.
//
// Deprecated: prefer SshAttestationStatement and/or TlsAttestationStatement.
teleport.attestation.v1.AttestationStatement attestation_statement = 16 [
(gogoproto.jsontag) = "attestation_statement,omitempty",
deprecated = true
];
// ClientLoginIP specifies IP address of the client for login, it will be written to the user's certificates.
string ClientLoginIP = 17 [(gogoproto.jsontag) = "client_login_ip,omitempty"];
// ClientUserAgent is the user agent of the Web browser, used for issuing
// a DeviceWebToken.
string ClientUserAgent = 18 [(gogoproto.jsontag) = "client_user_agent,omitempty"];
// SshPublicKey is an optional public key to use as the subject of an issued
// SSH cert in case of successful auth.
bytes ssh_public_key = 19 [(gogoproto.jsontag) = "ssh_pub_key,omitempty"];
// TlsPublicKey is an optional public key to use as the subject of an issued
// TLS cert in case of successful auth.
bytes tls_public_key = 20 [(gogoproto.jsontag) = "tls_pub_key,omitempty"];
// SshAttestationStatement is an attestation statement for the given SSH public key.
teleport.attestation.v1.AttestationStatement ssh_attestation_statement = 21 [(gogoproto.jsontag) = "ssh_attestation_statement,omitempty"];
// TlsAttestationStatement is an attestation statement for the given TLS public key.
teleport.attestation.v1.AttestationStatement tls_attestation_statement = 22 [(gogoproto.jsontag) = "tls_attestation_statement,omitempty"];
}

// SSOWarnings conveys a user-facing main message along with auxiliary warnings.
Expand Down
44 changes: 26 additions & 18 deletions api/types/github.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -338,30 +338,38 @@ func (r *GithubAuthRequest) Expiry() time.Time {

// Check makes sure the request is valid
func (r *GithubAuthRequest) Check() error {
if r.ConnectorID == "" {
switch {
case r.ConnectorID == "":
return trace.BadParameter("missing ConnectorID")
}
if r.StateToken == "" {
case r.StateToken == "":
return trace.BadParameter("missing StateToken")
// we could collapse these two checks into one, but the error message would become ambiguous.
case r.SSOTestFlow && r.ConnectorSpec == nil:
return trace.BadParameter("ConnectorSpec cannot be nil when SSOTestFlow is true")
case !r.SSOTestFlow && r.ConnectorSpec != nil:
return trace.BadParameter("ConnectorSpec must be nil when SSOTestFlow is false")
case len(r.PublicKey) != 0 && len(r.SshPublicKey) != 0:
return trace.BadParameter("illegal to set both PublicKey and SshPublicKey")
case len(r.PublicKey) != 0 && len(r.TlsPublicKey) != 0:
return trace.BadParameter("illegal to set both PublicKey and TlsPublicKey")
case r.AttestationStatement != nil && r.SshAttestationStatement != nil:
return trace.BadParameter("illegal to set both AttestationStatement and SshAttestationStatement")
case r.AttestationStatement != nil && r.TlsAttestationStatement != nil:
return trace.BadParameter("illegal to set both AttestationStatement and TlsAttestationStatement")
}
if len(r.PublicKey) != 0 {
_, _, _, _, err := ssh.ParseAuthorizedKey(r.PublicKey)
sshPubKey := r.PublicKey
if len(sshPubKey) == 0 {
sshPubKey = r.SshPublicKey
}
if len(sshPubKey) > 0 {
_, _, _, _, err := ssh.ParseAuthorizedKey(sshPubKey)
if err != nil {
return trace.BadParameter("bad PublicKey: %v", err)
}
if (r.CertTTL > defaults.MaxCertDuration) || (r.CertTTL < defaults.MinCertDuration) {
return trace.BadParameter("wrong CertTTL")
return trace.BadParameter("bad SSH public key: %v", err)
}
}

// we could collapse these two checks into one, but the error message would become ambiguous.
if r.SSOTestFlow && r.ConnectorSpec == nil {
return trace.BadParameter("ConnectorSpec cannot be nil when SSOTestFlow is true")
}

if !r.SSOTestFlow && r.ConnectorSpec != nil {
return trace.BadParameter("ConnectorSpec must be nil when SSOTestFlow is false")
if len(r.PublicKey)+len(r.SshPublicKey)+len(r.TlsPublicKey) > 0 &&
(r.CertTTL > defaults.MaxCertDuration || r.CertTTL < defaults.MinCertDuration) {
return trace.BadParameter("wrong CertTTL")
}

return nil
}
46 changes: 27 additions & 19 deletions api/types/oidc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -497,31 +497,39 @@ func (o *OIDCConnectorV3) GetClientRedirectSettings() *SSOClientRedirectSettings
}

// Check returns nil if all parameters are great, err otherwise
func (i *OIDCAuthRequest) Check() error {
if i.ConnectorID == "" {
func (r *OIDCAuthRequest) Check() error {
switch {
case r.ConnectorID == "":
return trace.BadParameter("ConnectorID: missing value")
}
if i.StateToken == "" {
case r.StateToken == "":
return trace.BadParameter("StateToken: missing value")
case len(r.PublicKey) != 0 && len(r.SshPublicKey) != 0:
return trace.BadParameter("illegal to set both PublicKey and SshPublicKey")
case len(r.PublicKey) != 0 && len(r.TlsPublicKey) != 0:
return trace.BadParameter("illegal to set both PublicKey and TlsPublicKey")
case r.AttestationStatement != nil && r.SshAttestationStatement != nil:
return trace.BadParameter("illegal to set both AttestationStatement and SshAttestationStatement")
case r.AttestationStatement != nil && r.TlsAttestationStatement != nil:
return trace.BadParameter("illegal to set both AttestationStatement and TlsAttestationStatement")
// we could collapse these two checks into one, but the error message would become ambiguous.
case r.SSOTestFlow && r.ConnectorSpec == nil:
return trace.BadParameter("ConnectorSpec cannot be nil when SSOTestFlow is true")
case !r.SSOTestFlow && r.ConnectorSpec != nil:
return trace.BadParameter("ConnectorSpec must be nil when SSOTestFlow is false")
}
if len(i.PublicKey) != 0 {
_, _, _, _, err := ssh.ParseAuthorizedKey(i.PublicKey)
sshPubKey := r.PublicKey
if len(sshPubKey) == 0 {
sshPubKey = r.SshPublicKey
}
if len(sshPubKey) > 0 {
_, _, _, _, err := ssh.ParseAuthorizedKey(sshPubKey)
if err != nil {
return trace.BadParameter("PublicKey: bad key: %v", err)
}
if (i.CertTTL > defaults.MaxCertDuration) || (i.CertTTL < defaults.MinCertDuration) {
return trace.BadParameter("CertTTL: wrong certificate TTL")
return trace.BadParameter("bad SSH public key: %v", err)
}
}

// we could collapse these two checks into one, but the error message would become ambiguous.
if i.SSOTestFlow && i.ConnectorSpec == nil {
return trace.BadParameter("ConnectorSpec cannot be nil when SSOTestFlow is true")
}

if !i.SSOTestFlow && i.ConnectorSpec != nil {
return trace.BadParameter("ConnectorSpec must be nil when SSOTestFlow is false")
if len(r.PublicKey)+len(r.SshPublicKey)+len(r.TlsPublicKey) > 0 &&
(r.CertTTL > defaults.MaxCertDuration || r.CertTTL < defaults.MinCertDuration) {
return trace.BadParameter("wrong CertTTL")
}

return nil
}
43 changes: 26 additions & 17 deletions api/types/saml.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,28 +433,37 @@ func (o *SAMLConnectorV2) CheckAndSetDefaults() error {
}

// Check returns nil if all parameters are great, err otherwise
func (i *SAMLAuthRequest) Check() error {
if i.ConnectorID == "" {
func (r *SAMLAuthRequest) Check() error {
switch {
case r.ConnectorID == "":
return trace.BadParameter("ConnectorID: missing value")
// we could collapse these two checks into one, but the error message would become ambiguous.
case r.SSOTestFlow && r.ConnectorSpec == nil:
return trace.BadParameter("ConnectorSpec cannot be nil when SSOTestFlow is true")
case !r.SSOTestFlow && r.ConnectorSpec != nil:
return trace.BadParameter("ConnectorSpec must be nil when SSOTestFlow is false")
case len(r.PublicKey) != 0 && len(r.SshPublicKey) != 0:
return trace.BadParameter("illegal to set both PublicKey and SshPublicKey")
case len(r.PublicKey) != 0 && len(r.TlsPublicKey) != 0:
return trace.BadParameter("illegal to set both PublicKey and TlsPublicKey")
case r.AttestationStatement != nil && r.SshAttestationStatement != nil:
return trace.BadParameter("illegal to set both AttestationStatement and SshAttestationStatement")
case r.AttestationStatement != nil && r.TlsAttestationStatement != nil:
return trace.BadParameter("illegal to set both AttestationStatement and TlsAttestationStatement")
}
if len(i.PublicKey) != 0 {
_, _, _, _, err := ssh.ParseAuthorizedKey(i.PublicKey)
sshPubKey := r.PublicKey
if len(sshPubKey) == 0 {
sshPubKey = r.SshPublicKey
}
if len(sshPubKey) > 0 {
_, _, _, _, err := ssh.ParseAuthorizedKey(sshPubKey)
if err != nil {
return trace.BadParameter("PublicKey: bad key: %v", err)
}
if (i.CertTTL > defaults.MaxCertDuration) || (i.CertTTL < defaults.MinCertDuration) {
return trace.BadParameter("CertTTL: wrong certificate TTL")
return trace.BadParameter("bad SSH public key: %v", err)
}
}

// we could collapse these two checks into one, but the error message would become ambiguous.
if i.SSOTestFlow && i.ConnectorSpec == nil {
return trace.BadParameter("ConnectorSpec cannot be nil when SSOTestFlow is true")
}

if !i.SSOTestFlow && i.ConnectorSpec != nil {
return trace.BadParameter("ConnectorSpec must be nil when SSOTestFlow is false")
if len(r.PublicKey)+len(r.SshPublicKey)+len(r.TlsPublicKey) > 0 &&
(r.CertTTL > defaults.MaxCertDuration || r.CertTTL < defaults.MinCertDuration) {
return trace.BadParameter("wrong CertTTL")
}

return nil
}
Loading

0 comments on commit acc08a3

Please sign in to comment.