From 164ec4591838ffb4d4a02e744876e4c2ed887f39 Mon Sep 17 00:00:00 2001 From: Tiago Silva Date: Wed, 28 Aug 2024 14:47:41 +0100 Subject: [PATCH] remove ability to store Jamf credentials in plugin specification (#45255) * remove possibility of storing jamf credentials in the jamf plugin spec This PR removes the `client_id`, `client_secret`, `username`, `password` fields from the plugin. Although we never aimed to use them as backend credentials storage, users trying to create the plugin programatically might end up setting them and having their Jamf credentials insecurely stored. Signed-off-by: Tiago Silva * handle code review * remove potential breaking change * handle code review --------- Signed-off-by: Tiago Silva --- lib/config/configuration.go | 12 +++- lib/config/configuration_test.go | 12 ++++ lib/config/fileconf.go | 54 ++++++++++------ lib/service/servicecfg/jamf.go | 43 ++++++++++++- lib/service/servicecfg/jamf_test.go | 96 +++++++++++++++++++++++++++++ 5 files changed, 194 insertions(+), 23 deletions(-) create mode 100644 lib/service/servicecfg/jamf_test.go diff --git a/lib/config/configuration.go b/lib/config/configuration.go index 92ec3edcda97b..53cf7dc5ff381 100644 --- a/lib/config/configuration.go +++ b/lib/config/configuration.go @@ -2925,14 +2925,20 @@ func applyJamfConfig(fc *FileConfig, cfg *servicecfg.Config) error { return nil } - jamfSpec, err := fc.Jamf.toJamfSpecV1() + creds, err := fc.Jamf.readJamfCredentials() + if err != nil { + return trace.Wrap(err) + } + + jamfSpec, err := fc.Jamf.toJamfSpecV1(creds) if err != nil { return trace.Wrap(err) } cfg.Jamf = servicecfg.JamfConfig{ - Spec: jamfSpec, - ExitOnSync: fc.Jamf.ExitOnSync, + Spec: jamfSpec, + ExitOnSync: fc.Jamf.ExitOnSync, + Credentials: creds, } return nil } diff --git a/lib/config/configuration_test.go b/lib/config/configuration_test.go index 33ea8baf0bfec..1ebf15fe1c166 100644 --- a/lib/config/configuration_test.go +++ b/lib/config/configuration_test.go @@ -3428,6 +3428,10 @@ jamf_service: Username: "llama", Password: password, }, + Credentials: &servicecfg.JamfCredentials{ + Username: "llama", + Password: password, + }, }, }, { @@ -3444,6 +3448,10 @@ jamf_service: ClientId: "llama-UUID", ClientSecret: password, }, + Credentials: &servicecfg.JamfCredentials{ + ClientID: "llama-UUID", + ClientSecret: password, + }, }, }, { @@ -3477,6 +3485,10 @@ jamf_service: {}, }, }, + Credentials: &servicecfg.JamfCredentials{ + Username: "llama", + Password: password, + }, ExitOnSync: true, }, }, diff --git a/lib/config/fileconf.go b/lib/config/fileconf.go index e16b72a74eef4..406eef1db63c0 100644 --- a/lib/config/fileconf.go +++ b/lib/config/fileconf.go @@ -2576,7 +2576,7 @@ type JamfInventoryEntry struct { PageSize int32 `yaml:"page_size,omitempty"` } -func (j *JamfService) toJamfSpecV1() (*types.JamfSpecV1, error) { +func (j *JamfService) toJamfSpecV1(creds *servicecfg.JamfCredentials) (*types.JamfSpecV1, error) { switch { case j == nil: return nil, trace.BadParameter("jamf_service is nil") @@ -2584,16 +2584,6 @@ func (j *JamfService) toJamfSpecV1() (*types.JamfSpecV1, error) { return nil, trace.BadParameter("jamf listen_addr not supported") } - // Read secrets. - password, err := readJamfPasswordFile(j.PasswordFile, "password_file") - if err != nil { - return nil, trace.Wrap(err) - } - clientSecret, err := readJamfPasswordFile(j.ClientSecretFile, "client_secret_file") - if err != nil { - return nil, trace.Wrap(err) - } - // Assemble spec. inventory := make([]*types.JamfInventoryEntry, len(j.Inventory)) for i, e := range j.Inventory { @@ -2606,23 +2596,49 @@ func (j *JamfService) toJamfSpecV1() (*types.JamfSpecV1, error) { } } spec := &types.JamfSpecV1{ - Enabled: j.Enabled(), - Name: j.Name, - SyncDelay: types.Duration(j.SyncDelay), - ApiEndpoint: j.APIEndpoint, + Enabled: j.Enabled(), + Name: j.Name, + SyncDelay: types.Duration(j.SyncDelay), + ApiEndpoint: j.APIEndpoint, + Inventory: inventory, + // TODO(tigrato): DELETE once we remove the fields from the config. + Username: creds.Username, + Password: creds.Password, + ClientId: creds.ClientID, + ClientSecret: creds.ClientSecret, + } + + // Validate. + if err := types.ValidateJamfSpecV1(spec); err != nil { + return nil, trace.BadParameter("jamf_service %v", err) + } + + return spec, nil +} + +func (j *JamfService) readJamfCredentials() (*servicecfg.JamfCredentials, error) { + password, err := readJamfPasswordFile(j.PasswordFile, "password_file") + if err != nil { + return nil, trace.Wrap(err) + } + clientSecret, err := readJamfPasswordFile(j.ClientSecretFile, "client_secret_file") + if err != nil { + return nil, trace.Wrap(err) + } + + creds := &servicecfg.JamfCredentials{ Username: j.Username, Password: password, - Inventory: inventory, - ClientId: j.ClientID, + ClientID: j.ClientID, ClientSecret: clientSecret, } // Validate. - if err := types.ValidateJamfSpecV1(spec); err != nil { + if err := servicecfg.ValidateJamfCredentials(creds); err != nil { return nil, trace.BadParameter("jamf_service %v", err) } - return spec, nil + return creds, nil } func readJamfPasswordFile(path, key string) (string, error) { diff --git a/lib/service/servicecfg/jamf.go b/lib/service/servicecfg/jamf.go index 5e31c7b81140b..007eea1d16697 100644 --- a/lib/service/servicecfg/jamf.go +++ b/lib/service/servicecfg/jamf.go @@ -18,12 +18,53 @@ package servicecfg -import "github.com/gravitational/teleport/api/types" +import ( + "github.com/gravitational/trace" + + "github.com/gravitational/teleport/api/types" +) + +// JamfCredentials is the credentials for the Jamf MDM service. +type JamfCredentials struct { + // Username is the Jamf API username. + // Username and password are used to acquire short-lived Jamf Pro API tokens. + // See https://developer.jamf.com/jamf-pro/docs/jamf-pro-api-overview. + // Prefer using client_id and client_secret. + // Either username+password or client_id+client_secret are required. + Username string + // Password is the Jamf API password. + // Username and password are used to acquire short-lived Jamf Pro API tokens. + // See https://developer.jamf.com/jamf-pro/docs/jamf-pro-api-overview. + // Prefer using client_id and client_secret. + // Either username+password or client_id+client_secret are required. + Password string + // ClientID is the Jamf API client ID. + // See https://developer.jamf.com/jamf-pro/docs/client-credentials. + // Either username+password or client_id+client_secret are required. + ClientID string + // ClientSecret is the Jamf API client secret. + // See https://developer.jamf.com/jamf-pro/docs/client-credentials. + // Either username+password or client_id+client_secret are required + ClientSecret string +} + +// ValidateJamfCredentials validates the Jamf credentials. +func ValidateJamfCredentials(j *JamfCredentials) error { + hasUserPass := j.Username != "" && j.Password != "" + hasAPICreds := j.ClientID != "" && j.ClientSecret != "" + switch { + case !hasUserPass && !hasAPICreds: + return trace.BadParameter("either username+password or clientID+clientSecret must be provided") + } + return nil +} // JamfConfig is the configuration for the Jamf MDM service. type JamfConfig struct { // Spec is the configuration spec. Spec *types.JamfSpecV1 + // Credentials are the Jamf API credentials. + Credentials *JamfCredentials // ExitOnSync controls whether the service performs a single sync operation // before exiting. ExitOnSync bool diff --git a/lib/service/servicecfg/jamf_test.go b/lib/service/servicecfg/jamf_test.go new file mode 100644 index 0000000000000..015ee8f20cfc3 --- /dev/null +++ b/lib/service/servicecfg/jamf_test.go @@ -0,0 +1,96 @@ +/* + * Teleport + * Copyright (C) 2024 Gravitational, Inc. + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License + * along with this program. If not, see . + */ + +package servicecfg + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestValidateJamfCredentials(t *testing.T) { + const expectedErr = "either username+password or clientID+clientSecret must be provided" + tests := []struct { + name string + creds *JamfCredentials + wantErr string + }{ + { + name: "valid credentials with username and password", + creds: &JamfCredentials{ + Username: "username", + Password: "password", + }, + }, + { + name: "valid credentials with client ID and client secret", + creds: &JamfCredentials{ + ClientID: "client-id", + ClientSecret: "client-secret", + }, + }, + { + name: "credentials with all fields set", + creds: &JamfCredentials{ + Username: "username", + Password: "password", + ClientID: "client-id", + ClientSecret: "client-secret", + }, + }, + { + name: "invalid credentials missing password", + creds: &JamfCredentials{ + Username: "username", + }, + wantErr: expectedErr, + }, + { + name: "invalid credentials missing username", + creds: &JamfCredentials{ + Password: "password", + }, + wantErr: expectedErr, + }, + { + name: "invalid credentials missing client secret", + creds: &JamfCredentials{ + ClientID: "id", + }, + wantErr: expectedErr, + }, + { + name: "invalid credentials missing client id", + creds: &JamfCredentials{ + ClientSecret: "secret", + }, + wantErr: expectedErr, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateJamfCredentials(tt.creds) + if tt.wantErr == "" { + require.NoError(t, err) + } else { + require.EqualError(t, err, tt.wantErr) + } + }) + } +}