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 <[email protected]> * handle code review * remove potential breaking change * handle code review --------- Signed-off-by: Tiago Silva <[email protected]>
gravitational · Aug 28, 2024 · 164ec45 · 164ec45
1 parent 2be0d82
commit 164ec45
Show file tree

Hide file tree

Showing 5 changed files with 194 additions and 23 deletions.
diff --git 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
@@ -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
@@ -2576,24 +2576,14 @@ 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")
 	case j.ListenAddress != "":
 		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
@@ -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
@@ -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 <http://www.gnu.org/licenses/>.
+ */
+
+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)
+			}
+		})
+	}
+}