diff --git a/internals/daemon/api_identities.go b/internals/daemon/api_identities.go index eb432a1a7..572bd4cfa 100644 --- a/internals/daemon/api_identities.go +++ b/internals/daemon/api_identities.go @@ -15,7 +15,10 @@ package daemon import ( + "crypto/x509" "encoding/json" + "encoding/pem" + "errors" "fmt" "net/http" @@ -23,46 +26,121 @@ import ( "github.com/canonical/pebble/internals/overlord/identities" ) +// apiIdentity exists so the API marshalling of an Identity excludes secrets. +type apiIdentity struct { + Access identities.Access `json:"access"` + Local *apiLocalIdentity `json:"local,omitempty"` + Basic *apiBasicIdentity `json:"basic,omitempty"` + Cert *apiCertIdentity `json:"cert,omitempty"` +} + +type apiLocalIdentity struct { + // Needs to be a pointer so we can distinguish between missing and zero (UID 0). + UserID *uint32 `json:"user-id"` +} + +type apiBasicIdentity struct { + Password string `json:"password"` +} + +type apiCertIdentity struct { + PEM string `json:"pem"` +} + +// When adding a new identity type, be sure to mask secrets here. +func identityToAPI(d *identities.Identity) *apiIdentity { + ai := &apiIdentity{ + Access: d.Access, + } + if d.Local != nil { + ai.Local = &apiLocalIdentity{UserID: &d.Local.UserID} + } + if d.Basic != nil { + ai.Basic = &apiBasicIdentity{Password: "*****"} + } + if d.Cert != nil { + // This isn't actually secret, it's a public key by design, but we + // replace it with ***** for consistency with the password field to + // avoid confusion for the user. We can show it in future if needed. + ai.Cert = &apiCertIdentity{PEM: "*****"} + } + return ai +} + +func identityFromAPI(ai *apiIdentity, name string) (*identities.Identity, error) { + if ai == nil { + return nil, nil + } + + identity := &identities.Identity{ + Access: ai.Access, + } + + if ai.Local != nil { + if ai.Local.UserID == nil { + return nil, errors.New("local identity must specify user-id") + } + identity.Local = &identities.LocalIdentity{UserID: *ai.Local.UserID} + } + if ai.Basic != nil { + identity.Basic = &identities.BasicIdentity{Password: ai.Basic.Password} + } + if ai.Cert != nil { + block, rest := pem.Decode([]byte(ai.Cert.PEM)) + if block == nil { + return nil, errors.New("cert identity must include a PEM-encoded certificate") + } + if len(rest) > 0 { + return nil, errors.New("cert identity cannot have extra data after the PEM block") + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return nil, fmt.Errorf("cannot parse certificate from cert identity: %w", err) + } + identity.Cert = &identities.CertIdentity{X509: cert} + } + + // Perform additional validation using the local Identity type. + err := identity.Validate(name) + if err != nil { + return nil, err + } + + return identity, nil +} + func v1GetIdentities(c *Command, r *http.Request, _ *UserState) Response { st := c.d.overlord.State() st.Lock() defer st.Unlock() identitiesMgr := c.d.overlord.IdentitiesManager() - identities := identitiesMgr.Identities() - return SyncResponse(identities) + idents := identitiesMgr.Identities() + + apiIdentities := make(map[string]*apiIdentity, len(idents)) + for name, identity := range idents { + apiIdentities[name] = identityToAPI(identity) + } + return SyncResponse(apiIdentities) } func v1PostIdentities(c *Command, r *http.Request, user *UserState) Response { var payload struct { - Action string `json:"action"` - Identities map[string]*identities.Identity `json:"identities"` + Action string `json:"action"` + Identities map[string]*apiIdentity `json:"identities"` } decoder := json.NewDecoder(r.Body) if err := decoder.Decode(&payload); err != nil { return BadRequest("cannot decode request body: %v", err) } - var identityNames map[string]struct{} - switch payload.Action { - case "add", "update": - for name, identity := range payload.Identities { - if identity == nil { - return BadRequest(`identity value for %q must not be null for %s operation`, name, payload.Action) - } - } - case "replace": - break - case "remove": - identityNames = make(map[string]struct{}) - for name, identity := range payload.Identities { - if identity != nil { - return BadRequest(`identity value for %q must be null for %s operation`, name, payload.Action) - } - identityNames[name] = struct{}{} + idents := make(map[string]*identities.Identity, len(payload.Identities)) + for name, apiIdent := range payload.Identities { + identity, err := identityFromAPI(apiIdent, name) + if err != nil { + return BadRequest("invalid identity for %q: %v", name, err) } - default: - return BadRequest(`invalid action %q, must be "add", "update", "replace", or "remove"`, payload.Action) + idents[name] = identity } st := c.d.overlord.State() @@ -74,21 +152,31 @@ func v1PostIdentities(c *Command, r *http.Request, user *UserState) Response { var err error switch payload.Action { case "add": - for name, identity := range payload.Identities { + for name, identity := range idents { + if identity == nil { + return BadRequest(`identity value for %q must not be null for add operation`, name) + } + } + for name, identity := range idents { logger.SecurityWarn(logger.SecurityUserCreated, fmt.Sprintf("%s,%s,%s", userString(user), name, identity.Access), fmt.Sprintf("Creating %s user %s", identity.Access, name)) } - err = identitiesMgr.AddIdentities(payload.Identities) + err = identitiesMgr.AddIdentities(idents) case "update": - for name, identity := range payload.Identities { + for name, identity := range idents { + if identity == nil { + return BadRequest(`identity value for %q must not be null for update operation`, name) + } + } + for name, identity := range idents { logger.SecurityWarn(logger.SecurityUserUpdated, fmt.Sprintf("%s,%s,%s", userString(user), name, identity.Access), fmt.Sprintf("Updating %s user %s", identity.Access, name)) } - err = identitiesMgr.UpdateIdentities(payload.Identities) + err = identitiesMgr.UpdateIdentities(idents) case "replace": - for name, identity := range payload.Identities { + for name, identity := range idents { if identity == nil { logger.SecurityWarn(logger.SecurityUserDeleted, fmt.Sprintf("%s,%s", userString(user), name), @@ -99,14 +187,23 @@ func v1PostIdentities(c *Command, r *http.Request, user *UserState) Response { fmt.Sprintf("Updating %s user %s", identity.Access, name)) } } - err = identitiesMgr.ReplaceIdentities(payload.Identities) + err = identitiesMgr.ReplaceIdentities(idents) case "remove": - for name := range payload.Identities { + identitiesToRemove := make(map[string]struct{}, len(idents)) + for name, identity := range idents { + if identity != nil { + return BadRequest(`identity value for %q must be null for remove operation`, name) + } + identitiesToRemove[name] = struct{}{} + } + for name := range identitiesToRemove { logger.SecurityWarn(logger.SecurityUserDeleted, fmt.Sprintf("%s,%s", userString(user), name), fmt.Sprintf("Deleting user %s", name)) } - err = identitiesMgr.RemoveIdentities(identityNames) + err = identitiesMgr.RemoveIdentities(identitiesToRemove) + default: + return BadRequest(`invalid action %q, must be "add", "update", "replace", or "remove"`, payload.Action) } if err != nil { return BadRequest("%v", err) diff --git a/internals/daemon/api_identities_test.go b/internals/daemon/api_identities_test.go index 40599fc3f..9b23e59f9 100644 --- a/internals/daemon/api_identities_test.go +++ b/internals/daemon/api_identities_test.go @@ -15,7 +15,10 @@ package daemon import ( + "crypto/x509" "encoding/json" + "encoding/pem" + "fmt" "net/http" "strings" @@ -25,6 +28,28 @@ import ( "github.com/canonical/pebble/internals/overlord/identities" ) +// Generated using `openssl req -new -x509 -out cert.pem -days 3650 -subj "/CN=canonical.com"` +const validPEMX509Cert = `-----BEGIN CERTIFICATE----- +MIIBRDCB96ADAgECAhROTkdEcgeil5/5NUNTq1ZRPDLiPTAFBgMrZXAwGDEWMBQG +A1UEAwwNY2Fub25pY2FsLmNvbTAeFw0yNTA5MDgxNTI2NTJaFw0zNTA5MDYxNTI2 +NTJaMBgxFjAUBgNVBAMMDWNhbm9uaWNhbC5jb20wKjAFBgMrZXADIQDtxRqb9EMe +ffcoJ0jNn9ys8uDFeHnQ6JRxgNFvomDTHqNTMFEwHQYDVR0OBBYEFI/oHjhG1A7F +3HM7McXP7w7CxtrwMB8GA1UdIwQYMBaAFI/oHjhG1A7F3HM7McXP7w7CxtrwMA8G +A1UdEwEB/wQFMAMBAf8wBQYDK2VwA0EA40v4eckaV7RBXyRb0sfcCcgCAGYtiCSD +jwXVTUH4HLpbhK0RAaEPOL4h5jm36CrWTkxzpbdCrIu4NgPLQKJ6Cw== +-----END CERTIFICATE----- +` + +// Generated using `openssl req -new -newkey ed25519 -out bad-cert.pem -nodes -subj "/CN=canonical.com"` +// This is a valid PEM block but not a valid X.509 certificate. +const testPEMPKCS10Req = `-----BEGIN CERTIFICATE REQUEST----- +MIGXMEsCAQAwGDEWMBQGA1UEAwwNY2Fub25pY2FsLmNvbTAqMAUGAytlcAMhADuu +TTkzIDS55kZukGFfsWM+kPug1hpJLVx4wKqr5eLNoAAwBQYDK2VwA0EA3QU93q5S +pV4RrgnD3G7kw2dg8fdJAZ/qn1bXToUzPy89uPMiAZIE+eHXBxzqTJ6GJrVY+2r7 +GV6pXv511MycDg== +-----END CERTIFICATE REQUEST----- +` + func (s *apiSuite) TestIdentities(c *C) { s.daemon(c) @@ -46,6 +71,10 @@ func (s *apiSuite) TestIdentities(c *C) { Password: "$6$F9cFSVEKyO4gB1Wh$8S1BSKsNkF.jBAixGc4W7l80OpfCNk65LZBDHBng3NAmbcHuMj4RIm7992rrJ8YA.SJ0hvm.vGk2z483am4Ym1", // "test" }, }, + "olivia": { + Access: identities.ReadAccess, + Cert: &identities.CertIdentity{X509: parseCert(c, validPEMX509Cert)}, + }, }) c.Assert(err, IsNil) st.Unlock() @@ -58,7 +87,7 @@ func (s *apiSuite) TestIdentities(c *C) { c.Check(rsp.Type, Equals, ResponseTypeSync) c.Check(rsp.Status, Equals, http.StatusOK) - identities, ok := rsp.Result.(map[string]*identities.Identity) + identities, ok := rsp.Result.(map[string]*apiIdentity) c.Assert(ok, Equals, true) data, err := json.MarshalIndent(identities, "", " ") @@ -82,6 +111,12 @@ func (s *apiSuite) TestIdentities(c *C) { "basic": { "password": "*****" } + }, + "olivia": { + "access": "read", + "cert": { + "pem": "*****" + } } }`[1:]) } @@ -92,7 +127,9 @@ func (s *apiSuite) TestAddIdentities(c *C) { s.daemon(c) - body := ` + jsonCert, err := json.Marshal(validPEMX509Cert) + c.Assert(err, IsNil) + body := fmt.Sprintf(` { "action": "add", "identities": { @@ -107,9 +144,21 @@ func (s *apiSuite) TestAddIdentities(c *C) { "local": { "user-id": 1000 } + }, + "nancy": { + "access": "metrics", + "basic": { + "password": "$6$F9cFSVEKyO4gB1Wh$8S1BSKsNkF.jBAixGc4W7l80OpfCNk65LZBDHBng3NAmbcHuMj4RIm7992rrJ8YA.SJ0hvm.vGk2z483am4Ym1" + } + }, + "olivia": { + "access": "read", + "cert": { + "pem": %s + } } } -}` +}`, jsonCert) rsp := s.postIdentities(c, body) c.Check(rsp.Type, Equals, ResponseTypeSync) c.Check(rsp.Status, Equals, http.StatusOK) @@ -129,11 +178,23 @@ func (s *apiSuite) TestAddIdentities(c *C) { Access: identities.AdminAccess, Local: &identities.LocalIdentity{UserID: 1000}, }, + "nancy": { + Name: "nancy", + Access: identities.MetricsAccess, + Basic: &identities.BasicIdentity{Password: "$6$F9cFSVEKyO4gB1Wh$8S1BSKsNkF.jBAixGc4W7l80OpfCNk65LZBDHBng3NAmbcHuMj4RIm7992rrJ8YA.SJ0hvm.vGk2z483am4Ym1"}, + }, + "olivia": { + Name: "olivia", + Access: identities.ReadAccess, + Cert: &identities.CertIdentity{X509: parseCert(c, validPEMX509Cert)}, + }, }) st.Unlock() ensureSecurityLog(c, logBuf.String(), "WARN", "user_created:,bob,read", "Creating read user bob") ensureSecurityLog(c, logBuf.String(), "WARN", "user_created:,mary,admin", "Creating admin user mary") + ensureSecurityLog(c, logBuf.String(), "WARN", "user_created:,nancy,metrics", "Creating metrics user nancy") + ensureSecurityLog(c, logBuf.String(), "WARN", "user_created:,olivia,read", "Creating read user olivia") } func (s *apiSuite) TestAddIdentitiesNull(c *C) { @@ -388,6 +449,61 @@ func (s *apiSuite) TestPostIdentitiesInvalidAction(c *C) { c.Assert(result.Message, Matches, `invalid action "foobar", must be "add", "update", "replace", or "remove"`) } +func (s *apiSuite) TestUnmarshalErrors(c *C) { + s.daemon(c) + + // Marshal a certificate request to test valid PEM but invalid X.509. + jsonCertReq, err := json.Marshal(testPEMPKCS10Req) + c.Assert(err, IsNil) + // Marshal a certificate with extra data after the PEM block. + jsonCertExtra, err := json.Marshal(validPEMX509Cert + "42") + c.Assert(err, IsNil) + + tests := []struct { + data string + error string + }{{ + data: `{"no-type": {"access": "admin"}}`, + error: `identity must have at least one type \("local", "basic", or "cert"\)`, + }, { + data: `{"invalid-access": {"access": "admin", "local": {}}}`, + error: `local identity must specify user-id`, + }, { + data: `{"invalid-access": {"access": "metrics", "basic": {}}}`, + error: `basic identity must specify password \(hashed\)`, + }, { + data: `{"invalid-access": {"access": "read", "cert": {}}}`, + error: `cert identity must include a PEM-encoded certificate`, + }, { + data: `{"invalid-access": {"access": "read", "cert": {"pem": "..."}}}`, + error: `cert identity must include a PEM-encoded certificate`, + }, { + data: fmt.Sprintf(`{"invalid-access": {"access": "read", "cert": {"pem": %s}}}`, jsonCertReq), + error: `cannot parse certificate from cert identity: x509: .*`, + }, { + data: fmt.Sprintf(`{"invalid-access": {"access": "read", "cert": {"pem": %s}}}`, jsonCertExtra), + error: `cert identity cannot have extra data after the PEM block`, + }, { + data: `{"invalid-access": {"access": "foo", "local": {"user-id": 42}}}`, + error: `invalid access value "foo", must be "admin", "read", "metrics", or "untrusted"`, + }, { + data: `{"invalid-access": {"local": {"user-id": 42}}}`, + error: `access value must be specified \("admin", "read", "metrics", or "untrusted"\)`, + }} + for _, test := range tests { + c.Logf("Input data: %s", test.data) + + body := fmt.Sprintf(`{"action": "foobar", "identities": %s}`, test.data) + + rsp := s.postIdentities(c, body) + c.Check(rsp.Type, Equals, ResponseTypeError) + c.Check(rsp.Status, Equals, http.StatusBadRequest) + result, ok := rsp.Result.(*errorResult) + c.Assert(ok, Equals, true) + c.Check(result.Message, Matches, ".*: "+test.error) + } +} + func (s *apiSuite) postIdentities(c *C, body string) *resp { req, err := http.NewRequest("POST", "/v1/identities", strings.NewReader(body)) c.Assert(err, IsNil) @@ -396,3 +512,11 @@ func (s *apiSuite) postIdentities(c *C, body string) *resp { c.Assert(ok, Equals, true) return rsp } + +func parseCert(c *C, pemBlock string) *x509.Certificate { + block, _ := pem.Decode([]byte(pemBlock)) + c.Assert(block, NotNil) + cert, _ := x509.ParseCertificate(block.Bytes) + c.Assert(cert, NotNil) + return cert +} diff --git a/internals/overlord/identities/identities.go b/internals/overlord/identities/identities.go index bc3512019..f8a2607ee 100644 --- a/internals/overlord/identities/identities.go +++ b/internals/overlord/identities/identities.go @@ -52,14 +52,13 @@ func NewManager(st *state.State) (*Manager, error) { defer m.state.Unlock() // Read existing identities from state, if any. - var marshalled map[string]*marshalledIdentity - err := st.Get(identitiesKey, &marshalled) + err := st.Get(identitiesKey, &m.identities) if err != nil && !errors.Is(err, state.ErrNoState) { return nil, err } - m.identities, err = unmarshalIdentities(marshalled) - if err != nil { - return nil, err + // Populate the Name field for each identity from the JSON object key. + for name, identity := range m.identities { + identity.Name = name } return m, nil @@ -70,15 +69,18 @@ func (m *Manager) Ensure() error { } // Identity holds the configuration of a single identity. +// +// IMPORTANT: When adding a new identity type, if there's sensitive fields in it +// (like passwords), be sure to omit it from API marshalling in api_identities.go. type Identity struct { - Name string - Access Access + Name string `json:"-"` + Access Access `json:"access"` // One or more of the following type-specific configuration fields must be // non-nil. - Local *LocalIdentity - Basic *BasicIdentity - Cert *CertIdentity + Local *LocalIdentity `json:"local,omitempty"` + Basic *BasicIdentity `json:"basic,omitempty"` + Cert *CertIdentity `json:"cert,omitempty"` } // Access defines the access level for an identity. @@ -94,14 +96,14 @@ const ( // LocalIdentity holds identity configuration specific to the "local" type // (for ucrednet/UID authentication). type LocalIdentity struct { - UserID uint32 + UserID uint32 `json:"user-id"` } // BasicIdentity holds identity configuration specific to the "basic" type // (for HTTP basic authentication). type BasicIdentity struct { // Password holds the user's sha512-crypt-hashed password. - Password string + Password string `json:"password"` } // Certificate identity represents the client in an mTLS connection. We @@ -110,11 +112,44 @@ type CertIdentity struct { X509 *x509.Certificate } +type marshalledCertIdentity struct { + PEM string `json:"pem"` +} + +func (c *CertIdentity) MarshalJSON() ([]byte, error) { + pemBlock := &pem.Block{ + Type: "CERTIFICATE", + Bytes: c.X509.Raw, + } + marshalled := marshalledCertIdentity{ + PEM: string(pem.EncodeToMemory(pemBlock)), + } + return json.Marshal(marshalled) +} + +func (c *CertIdentity) UnmarshalJSON(data []byte) error { + var unmarshalled marshalledCertIdentity + err := json.Unmarshal(data, &unmarshalled) + if err != nil { + return err + } + block, _ := pem.Decode([]byte(unmarshalled.PEM)) + if block == nil { + return errors.New("cert identity must include a PEM-encoded certificate") + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + return fmt.Errorf("cannot parse certificate from cert identity: %w", err) + } + c.X509 = cert + return nil +} + // This is used to ensure we send a well-formed identity Name. var identityNameRegexp = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9_\-]*$`) -// validate checks that the identity is valid, returning an error if not. -func (d *Identity) validate(name string) error { +// Validate checks that the identity's fields (and name) are valid, returning an error if not. +func (d *Identity) Validate(name string) error { if d == nil { return errors.New("identity must not be nil") } @@ -123,15 +158,6 @@ func (d *Identity) validate(name string) error { return fmt.Errorf("identity name %q invalid: must start with an alphabetic character and only contain alphanumeric characters, underscore, and hyphen", d.Name) } - return d.validateAccess() -} - -// validateAccess checks that the identity's access and type are valid, returning an error if not. -func (d *Identity) validateAccess() error { - if d == nil { - return errors.New("identity must not be nil") - } - switch d.Access { case AdminAccess, ReadAccess, MetricsAccess, UntrustedAccess: case "": @@ -165,93 +191,6 @@ func (d *Identity) validateAccess() error { return nil } -// apiIdentity exists so the default JSON marshalling of an Identity (used -// for API responses) excludes secrets. The marshalledIdentity type is used -// for saving secrets in state. -type apiIdentity struct { - Access string `json:"access"` - Local *apiLocalIdentity `json:"local,omitempty"` - Basic *apiBasicIdentity `json:"basic,omitempty"` - Cert *apiCertIdentity `json:"cert,omitempty"` -} - -type apiLocalIdentity struct { - UserID *uint32 `json:"user-id"` -} - -type apiBasicIdentity struct { - Password string `json:"password"` -} - -type apiCertIdentity struct { - PEM string `json:"pem"` -} - -// IMPORTANT NOTE: be sure to exclude secrets when adding to this! -func (d *Identity) MarshalJSON() ([]byte, error) { - ai := apiIdentity{ - Access: string(d.Access), - } - if d.Local != nil { - ai.Local = &apiLocalIdentity{UserID: &d.Local.UserID} - } - if d.Basic != nil { - ai.Basic = &apiBasicIdentity{Password: "*****"} - } - if d.Cert != nil { - // This isn't actually secret, it's a public key by design, but we - // replace it with ***** for consistency with the password field to - // avoid confusion for the user. We can show it in future if needed. - ai.Cert = &apiCertIdentity{PEM: "*****"} - } - return json.Marshal(ai) -} - -func (d *Identity) UnmarshalJSON(data []byte) error { - var ai apiIdentity - err := json.Unmarshal(data, &ai) - if err != nil { - return err - } - - identity := Identity{ - Access: Access(ai.Access), - } - - if ai.Local != nil { - if ai.Local.UserID == nil { - return errors.New("local identity must specify user-id") - } - identity.Local = &LocalIdentity{UserID: *ai.Local.UserID} - } - if ai.Basic != nil { - identity.Basic = &BasicIdentity{Password: ai.Basic.Password} - } - if ai.Cert != nil { - block, rest := pem.Decode([]byte(ai.Cert.PEM)) - if block == nil { - return errors.New("cert identity must include a PEM-encoded certificate") - } - if len(rest) > 0 { - return errors.New("cert identity cannot have extra data after the PEM block") - } - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return fmt.Errorf("cannot parse certificate from cert identity: %w", err) - } - identity.Cert = &CertIdentity{X509: cert} - } - - // Perform additional validation using the local Identity type. - err = identity.validateAccess() - if err != nil { - return err - } - - *d = identity - return nil -} - // AddIdentities adds the given identities to the system. It's an error if any // of the named identities already exist. // @@ -263,7 +202,7 @@ func (m *Manager) AddIdentities(identities map[string]*Identity) error { if _, ok := m.identities[name]; ok { existing = append(existing, name) } - err := identity.validate(name) + err := identity.Validate(name) if err != nil { return fmt.Errorf("identity %q invalid: %w", name, err) } @@ -285,7 +224,7 @@ func (m *Manager) AddIdentities(identities map[string]*Identity) error { } m.identities = newIdentities - m.state.Set(identitiesKey, marshalledIdentities(newIdentities)) + m.state.Set(identitiesKey, newIdentities) return nil } @@ -300,7 +239,7 @@ func (m *Manager) UpdateIdentities(identities map[string]*Identity) error { if _, ok := m.identities[name]; !ok { missing = append(missing, name) } - err := identity.validate(name) + err := identity.Validate(name) if err != nil { return fmt.Errorf("identity %q invalid: %w", name, err) } @@ -322,7 +261,7 @@ func (m *Manager) UpdateIdentities(identities map[string]*Identity) error { } m.identities = newIdentities - m.state.Set(identitiesKey, marshalledIdentities(newIdentities)) + m.state.Set(identitiesKey, newIdentities) return nil } @@ -334,7 +273,7 @@ func (m *Manager) UpdateIdentities(identities map[string]*Identity) error { func (m *Manager) ReplaceIdentities(identities map[string]*Identity) error { for name, identity := range identities { if identity != nil { - err := identity.validate(name) + err := identity.Validate(name) if err != nil { return fmt.Errorf("identity %q invalid: %w", name, err) } @@ -357,7 +296,7 @@ func (m *Manager) ReplaceIdentities(identities map[string]*Identity) error { } m.identities = newIdentities - m.state.Set(identitiesKey, marshalledIdentities(newIdentities)) + m.state.Set(identitiesKey, newIdentities) return nil } @@ -381,7 +320,7 @@ func (m *Manager) RemoveIdentities(identities map[string]struct{}) error { for name := range identities { delete(m.identities, name) } - m.state.Set(identitiesKey, marshalledIdentities(m.identities)) + m.state.Set(identitiesKey, m.identities) return nil } diff --git a/internals/overlord/identities/identities_test.go b/internals/overlord/identities/identities_test.go index fef11b4a6..b4274e4d2 100644 --- a/internals/overlord/identities/identities_test.go +++ b/internals/overlord/identities/identities_test.go @@ -19,7 +19,6 @@ import ( "crypto/x509" "encoding/json" "encoding/pem" - "fmt" "testing" . "gopkg.in/check.v1" @@ -56,177 +55,6 @@ mHLySscsVEgGwncFhL/9UW5iZl/tO/o+WiyVd/K4Vk0Yrp6uggA= -----END CERTIFICATE----- ` -// Generated using `openssl req -new -newkey ed25519 -out bad-cert.pem -nodes -subj "/CN=canonical.com"` -// This is a valid PEM block but not a valid X.509 certificate. -const testPEMPKCS10Req = `-----BEGIN CERTIFICATE REQUEST----- -MIGXMEsCAQAwGDEWMBQGA1UEAwwNY2Fub25pY2FsLmNvbTAqMAUGAytlcAMhADuu -TTkzIDS55kZukGFfsWM+kPug1hpJLVx4wKqr5eLNoAAwBQYDK2VwA0EA3QU93q5S -pV4RrgnD3G7kw2dg8fdJAZ/qn1bXToUzPy89uPMiAZIE+eHXBxzqTJ6GJrVY+2r7 -GV6pXv511MycDg== ------END CERTIFICATE REQUEST----- -` - -// IMPORTANT NOTE: be sure secrets aren't included when adding to this! -func (s *identitiesSuite) TestMarshalAPI(c *C) { - st := state.New(nil) - mgr, err := identities.NewManager(st) - c.Assert(err, IsNil) - - st.Lock() - defer st.Unlock() - - err = mgr.AddIdentities(map[string]*identities.Identity{ - "bob": { - Access: identities.ReadAccess, - Local: &identities.LocalIdentity{UserID: 42}, - }, - "mary": { - Access: identities.AdminAccess, - Local: &identities.LocalIdentity{UserID: 1000}, - }, - "nancy": { - Access: identities.MetricsAccess, - Basic: &identities.BasicIdentity{Password: "hash"}, - }, - "olivia": { - Access: identities.ReadAccess, - Cert: &identities.CertIdentity{X509: parseCert(c, validPEMX509Cert)}, - }, - }) - c.Assert(err, IsNil) - - identities := mgr.Identities() - data, err := json.MarshalIndent(identities, "", " ") - c.Assert(err, IsNil) - c.Assert(string(data), Equals, ` -{ - "bob": { - "access": "read", - "local": { - "user-id": 42 - } - }, - "mary": { - "access": "admin", - "local": { - "user-id": 1000 - } - }, - "nancy": { - "access": "metrics", - "basic": { - "password": "*****" - } - }, - "olivia": { - "access": "read", - "cert": { - "pem": "*****" - } - } -}`[1:]) -} - -func (s *identitiesSuite) TestUnmarshalAPI(c *C) { - jsonCert, err := json.Marshal(validPEMX509Cert) - c.Assert(err, IsNil) - data := fmt.Appendf(nil, ` -{ - "bob": { - "access": "read", - "local": { - "user-id": 42 - } - }, - "mary": { - "access": "admin", - "local": { - "user-id": 1000 - } - }, - "nancy": { - "access": "metrics", - "basic": { - "password": "hash" - } - }, - "olivia": { - "access": "read", - "cert": { - "pem": %s - } - } -}`, jsonCert) - var idents map[string]*identities.Identity - err = json.Unmarshal(data, &idents) - c.Assert(err, IsNil) - c.Assert(idents, DeepEquals, map[string]*identities.Identity{ - "bob": { - Access: identities.ReadAccess, - Local: &identities.LocalIdentity{UserID: 42}, - }, - "mary": { - Access: identities.AdminAccess, - Local: &identities.LocalIdentity{UserID: 1000}, - }, - "nancy": { - Access: identities.MetricsAccess, - Basic: &identities.BasicIdentity{Password: "hash"}, - }, - "olivia": { - Access: identities.ReadAccess, - Cert: &identities.CertIdentity{X509: parseCert(c, validPEMX509Cert)}, - }, - }) -} - -func (s *identitiesSuite) TestUnmarshalAPIErrors(c *C) { - // Marshal a certificate request to test valid PEM but invalid X.509. - jsonCertReq, err := json.Marshal(testPEMPKCS10Req) - c.Assert(err, IsNil) - // Marshal a certificate with extra data after the PEM block. - jsonCertExtra, err := json.Marshal(validPEMX509Cert + "42") - c.Assert(err, IsNil) - - tests := []struct { - data string - error string - }{{ - data: `{"no-type": {"access": "admin"}}`, - error: `identity must have at least one type \("local", "basic", or "cert"\)`, - }, { - data: `{"invalid-access": {"access": "admin", "local": {}}}`, - error: `local identity must specify user-id`, - }, { - data: `{"invalid-access": {"access": "metrics", "basic": {}}}`, - error: `basic identity must specify password \(hashed\)`, - }, { - data: `{"invalid-access": {"access": "read", "cert": {}}}`, - error: `cert identity must include a PEM-encoded certificate`, - }, { - data: `{"invalid-access": {"access": "read", "cert": {"pem": "..."}}}`, - error: `cert identity must include a PEM-encoded certificate`, - }, { - data: fmt.Sprintf(`{"invalid-access": {"access": "read", "cert": {"pem": %s}}}`, jsonCertReq), - error: `cannot parse certificate from cert identity: x509: .*`, - }, { - data: fmt.Sprintf(`{"invalid-access": {"access": "read", "cert": {"pem": %s}}}`, jsonCertExtra), - error: `cert identity cannot have extra data after the PEM block`, - }, { - data: `{"invalid-access": {"access": "foo", "local": {"user-id": 42}}}`, - error: `invalid access value "foo", must be "admin", "read", "metrics", or "untrusted"`, - }, { - data: `{"invalid-access": {"local": {"user-id": 42}}}`, - error: `access value must be specified \("admin", "read", "metrics", or "untrusted"\)`, - }} - for _, test := range tests { - c.Logf("Input data: %s", test.data) - var identities map[string]*identities.Identity - err := json.Unmarshal([]byte(test.data), &identities) - c.Check(err, ErrorMatches, test.error) - } -} - func (s *identitiesSuite) TestMarshalState(c *C) { st := state.New(nil) mgr, err := identities.NewManager(st) diff --git a/internals/overlord/identities/state.go b/internals/overlord/identities/state.go deleted file mode 100644 index 36ab323de..000000000 --- a/internals/overlord/identities/state.go +++ /dev/null @@ -1,76 +0,0 @@ -package identities - -import ( - "crypto/x509" - "encoding/pem" - "fmt" -) - -// marshalledIdentity is used specifically for marshalling to the state -// database file. Unlike apiIdentity, it should include secrets. -type marshalledIdentity struct { - Access string `json:"access"` - Local *marshalledLocalIdentity `json:"local,omitempty"` - Basic *marshalledBasicIdentity `json:"basic,omitempty"` - Cert *marshalledCertIdentity `json:"cert,omitempty"` -} - -type marshalledLocalIdentity struct { - UserID uint32 `json:"user-id"` -} - -type marshalledBasicIdentity struct { - Password string `json:"password"` -} - -type marshalledCertIdentity struct { - PEM string `json:"pem"` -} - -func marshalledIdentities(identities map[string]*Identity) map[string]*marshalledIdentity { - marshalled := make(map[string]*marshalledIdentity, len(identities)) - for name, identity := range identities { - marshalled[name] = &marshalledIdentity{ - Access: string(identity.Access), - } - if identity.Local != nil { - marshalled[name].Local = &marshalledLocalIdentity{UserID: identity.Local.UserID} - } - if identity.Basic != nil { - marshalled[name].Basic = &marshalledBasicIdentity{Password: identity.Basic.Password} - } - if identity.Cert != nil { - pemBlock := &pem.Block{ - Type: "CERTIFICATE", - Bytes: identity.Cert.X509.Raw, - } - marshalled[name].Cert = &marshalledCertIdentity{PEM: string(pem.EncodeToMemory(pemBlock))} - } - } - return marshalled -} - -func unmarshalIdentities(marshalled map[string]*marshalledIdentity) (map[string]*Identity, error) { - identities := make(map[string]*Identity, len(marshalled)) - for name, mi := range marshalled { - identities[name] = &Identity{ - Name: name, - Access: Access(mi.Access), - } - if mi.Local != nil { - identities[name].Local = &LocalIdentity{UserID: mi.Local.UserID} - } - if mi.Basic != nil { - identities[name].Basic = &BasicIdentity{Password: mi.Basic.Password} - } - if mi.Cert != nil { - block, _ := pem.Decode([]byte(mi.Cert.PEM)) - cert, err := x509.ParseCertificate(block.Bytes) - if err != nil { - return nil, fmt.Errorf("cannot parse certificate from cert identity: %w", err) - } - identities[name].Cert = &CertIdentity{X509: cert} - } - } - return identities, nil -}