Skip to content

Commit a5fab73

Browse files
authored
Merge pull request #147 from datum-cloud/fix/146-macos-keychain-size-limit
fix: unbreak machine account login on macOS
2 parents d4ab844 + dcffde8 commit a5fab73

File tree

5 files changed

+191
-9
lines changed

5 files changed

+191
-9
lines changed

internal/authutil/credentials.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,14 @@ type MachineAccountState struct {
3636
ClientEmail string `json:"client_email"`
3737
ClientID string `json:"client_id"`
3838
PrivateKeyID string `json:"private_key_id"`
39-
PrivateKey string `json:"private_key"`
39+
PrivateKey string `json:"private_key,omitempty"`
4040
TokenURI string `json:"token_uri"`
4141
Scope string `json:"scope,omitempty"`
42+
// PrivateKeyPath is the path to an on-disk file containing the PEM-encoded
43+
// private key. Used when the key is too large to store in the keyring (e.g.
44+
// on macOS where the Keychain has a per-item size limit). If non-empty, the
45+
// token source reads the key from this path instead of PrivateKey.
46+
PrivateKeyPath string `json:"private_key_path,omitempty"`
4247
}
4348

4449
// StoredCredentials holds all necessary information for a single authenticated session.
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
package authutil
2+
3+
import (
4+
"crypto/sha256"
5+
"encoding/hex"
6+
"errors"
7+
"fmt"
8+
"os"
9+
"path/filepath"
10+
)
11+
12+
// machineAccountKeyDir returns the directory used to store machine account PEM key files.
13+
// It does not create the directory.
14+
func machineAccountKeyDir() (string, error) {
15+
configDir, err := os.UserConfigDir()
16+
if err != nil {
17+
return "", fmt.Errorf("failed to determine user config directory: %w", err)
18+
}
19+
return filepath.Join(configDir, "datumctl", "machine-accounts"), nil
20+
}
21+
22+
// MachineAccountKeyFilePath returns the on-disk path where the PEM key for
23+
// the given userKey is stored. It does not create the directory.
24+
func MachineAccountKeyFilePath(userKey string) (string, error) {
25+
dir, err := machineAccountKeyDir()
26+
if err != nil {
27+
return "", err
28+
}
29+
sum := sha256.Sum256([]byte(userKey))
30+
filename := hex.EncodeToString(sum[:]) + ".pem"
31+
return filepath.Join(dir, filename), nil
32+
}
33+
34+
// WriteMachineAccountKeyFile atomically writes the PEM key to disk for the
35+
// given userKey and returns the absolute path. Creates the parent directory
36+
// with mode 0700 if needed. The file is written with mode 0600.
37+
func WriteMachineAccountKeyFile(userKey, pemKey string) (string, error) {
38+
dir, err := machineAccountKeyDir()
39+
if err != nil {
40+
return "", err
41+
}
42+
43+
if err := os.MkdirAll(dir, 0700); err != nil {
44+
return "", fmt.Errorf("failed to create machine account key directory %s: %w", dir, err)
45+
}
46+
47+
// Tighten perms on the directory even when it already existed with looser ones.
48+
if err := os.Chmod(dir, 0700); err != nil && !errors.Is(err, os.ErrNotExist) {
49+
return "", fmt.Errorf("failed to set permissions on machine account key directory %s: %w", dir, err)
50+
}
51+
52+
destPath, err := MachineAccountKeyFilePath(userKey)
53+
if err != nil {
54+
return "", err
55+
}
56+
57+
// Use a unique temp filename so concurrent logins for the same account
58+
// do not race on the same .tmp path and corrupt each other's writes.
59+
tmpFile, err := os.CreateTemp(dir, filepath.Base(destPath)+".tmp.*")
60+
if err != nil {
61+
return "", fmt.Errorf("failed to create temp file for machine account key in %s: %w", dir, err)
62+
}
63+
tmpName := tmpFile.Name()
64+
65+
// Ensure the temp file is removed on any error path. After a successful
66+
// Rename the file no longer exists at tmpName, so Remove is a no-op.
67+
var writeErr error
68+
defer func() {
69+
if writeErr != nil {
70+
_ = os.Remove(tmpName)
71+
}
72+
}()
73+
74+
// Be explicit about mode even though CreateTemp already uses 0600.
75+
if writeErr = tmpFile.Chmod(0600); writeErr != nil {
76+
_ = tmpFile.Close()
77+
writeErr = fmt.Errorf("failed to set permissions on temp key file %s: %w", tmpName, writeErr)
78+
return "", writeErr
79+
}
80+
81+
if _, writeErr = tmpFile.Write([]byte(pemKey)); writeErr != nil {
82+
_ = tmpFile.Close()
83+
writeErr = fmt.Errorf("failed to write machine account key to %s: %w", tmpName, writeErr)
84+
return "", writeErr
85+
}
86+
87+
if writeErr = tmpFile.Close(); writeErr != nil {
88+
writeErr = fmt.Errorf("failed to close temp key file %s: %w", tmpName, writeErr)
89+
return "", writeErr
90+
}
91+
92+
if writeErr = os.Rename(tmpName, destPath); writeErr != nil {
93+
writeErr = fmt.Errorf("failed to move machine account key to %s: %w", destPath, writeErr)
94+
return "", writeErr
95+
}
96+
97+
return destPath, nil
98+
}
99+
100+
// ReadMachineAccountKeyFile reads a PEM key from the given path.
101+
func ReadMachineAccountKeyFile(path string) (string, error) {
102+
data, err := os.ReadFile(path)
103+
if err != nil {
104+
return "", fmt.Errorf("failed to read machine account key file %s: %w", path, err)
105+
}
106+
return string(data), nil
107+
}
108+
109+
// RemoveMachineAccountKeyFile deletes the PEM key file for the given userKey.
110+
// Returns nil if the file does not exist.
111+
func RemoveMachineAccountKeyFile(userKey string) error {
112+
path, err := MachineAccountKeyFilePath(userKey)
113+
if err != nil {
114+
return err
115+
}
116+
if err := os.Remove(path); err != nil && !os.IsNotExist(err) {
117+
return fmt.Errorf("failed to remove machine account key file %s: %w", path, err)
118+
}
119+
return nil
120+
}

internal/authutil/machineaccount.go

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -194,7 +194,31 @@ func (m *machineAccountTokenSource) Token() (*oauth2.Token, error) {
194194
}
195195

196196
ma := m.creds.MachineAccount
197-
signedJWT, err := MintJWT(ma.ClientID, ma.PrivateKeyID, ma.PrivateKey, ma.TokenURI)
197+
198+
// Resolve the PEM key. New sessions store the key on disk (PrivateKeyPath)
199+
// to stay within the macOS Keychain per-item size limit; older sessions
200+
// (Linux, pre-fix) may still have the key inline in PrivateKey.
201+
pemKey := ma.PrivateKey
202+
if pemKey == "" && ma.PrivateKeyPath != "" {
203+
var readErr error
204+
pemKey, readErr = ReadMachineAccountKeyFile(ma.PrivateKeyPath)
205+
if readErr != nil {
206+
return nil, customerrors.WrapUserErrorWithHint(
207+
"failed to read machine account private key from "+ma.PrivateKeyPath,
208+
"re-run 'datumctl auth login --credentials <file>'; you may need to download a new machine account credentials file from the Datum portal if the original is no longer available",
209+
readErr,
210+
)
211+
}
212+
}
213+
if pemKey == "" {
214+
return nil, customerrors.WrapUserErrorWithHint(
215+
"machine account session is missing its private key",
216+
"re-run 'datumctl auth login --credentials <file>'; you may need to download a new machine account credentials file from the Datum portal if the original is no longer available",
217+
nil,
218+
)
219+
}
220+
221+
signedJWT, err := MintJWT(ma.ClientID, ma.PrivateKeyID, pemKey, ma.TokenURI)
198222
if err != nil {
199223
return nil, customerrors.WrapUserErrorWithHint(
200224
"Failed to mint JWT for machine account authentication.",

internal/cmd/auth/logout.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,11 @@ func logoutSingleUser(userKeyToLogout string) error {
9191
if err := keyring.Delete(authutil.ServiceName, userKeyToLogout); err != nil && !errors.Is(err, keyring.ErrNotFound) {
9292
fmt.Printf("Warning: attempt to delete potential stray key for %s failed: %v\n", userKeyToLogout, err)
9393
}
94+
// Also remove any stray on-disk PEM file. This is the exact cleanup path
95+
// users hit after a failed login left crypto material behind (issue #146).
96+
if removeErr := authutil.RemoveMachineAccountKeyFile(userKeyToLogout); removeErr != nil {
97+
fmt.Printf("Warning: failed to remove machine account key file for '%s': %v\n", userKeyToLogout, removeErr)
98+
}
9499
return nil
95100
}
96101

@@ -100,6 +105,14 @@ func logoutSingleUser(userKeyToLogout string) error {
100105
fmt.Printf("Warning: failed to delete credentials for user '%s' from keyring: %v\n", userKeyToLogout, err)
101106
}
102107

108+
// Remove the on-disk PEM key file for machine account sessions.
109+
// This is a best-effort cleanup: ignore "not found" (interactive sessions
110+
// never write a file) and only warn on other errors since the keyring entry
111+
// is already gone.
112+
if removeErr := authutil.RemoveMachineAccountKeyFile(userKeyToLogout); removeErr != nil {
113+
fmt.Printf("Warning: failed to remove machine account key file for '%s': %v\n", userKeyToLogout, removeErr)
114+
}
115+
103116
// 4. Update and save the known users list
104117
updatedJSON, err := json.Marshal(updatedKnownUsers)
105118
if err != nil {
@@ -164,6 +177,11 @@ func logoutAllUsers() error {
164177
fmt.Printf("Warning: failed to delete credentials for user '%s' from keyring: %v\n", userKey, err)
165178
logoutErrors = true // Mark that at least one error occurred
166179
}
180+
181+
// Remove the on-disk PEM key file for machine account sessions (best-effort).
182+
if removeErr := authutil.RemoveMachineAccountKeyFile(userKey); removeErr != nil {
183+
fmt.Printf("Warning: failed to remove machine account key file for '%s': %v\n", userKey, removeErr)
184+
}
167185
}
168186

169187
// 3. Delete the known users list itself

internal/cmd/auth/machine_account_login.go

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -115,6 +115,20 @@ func runMachineAccountLogin(ctx context.Context, credentialsPath, hostname, apiH
115115
displayName = creds.ClientID
116116
}
117117

118+
// Use client_email as the keyring key when available; fall back to client_id.
119+
userKey := creds.ClientEmail
120+
if userKey == "" {
121+
userKey = creds.ClientID
122+
}
123+
124+
// Write the PEM private key to disk to keep the keyring blob small.
125+
// On macOS the Keychain has a per-item size limit (~4 KB); embedding the
126+
// PEM (~2.5 KB) alongside the access token pushes the blob over the limit.
127+
keyFilePath, err := authutil.WriteMachineAccountKeyFile(userKey, creds.PrivateKey)
128+
if err != nil {
129+
return fmt.Errorf("failed to write machine account private key to disk: %w", err)
130+
}
131+
118132
stored := authutil.StoredCredentials{
119133
Hostname: hostname,
120134
APIHostname: finalAPIHostname,
@@ -129,7 +143,9 @@ func runMachineAccountLogin(ctx context.Context, credentialsPath, hostname, apiH
129143
ClientEmail: creds.ClientEmail,
130144
ClientID: creds.ClientID,
131145
PrivateKeyID: creds.PrivateKeyID,
132-
PrivateKey: creds.PrivateKey,
146+
// PrivateKey is intentionally left empty; the key lives on disk at
147+
// PrivateKeyPath so the keyring blob stays under the macOS size limit.
148+
PrivateKeyPath: keyFilePath,
133149
// Store the discovered token URI and resolved scope so that the
134150
// machineAccountTokenSource can refresh tokens without re-reading
135151
// the credentials file.
@@ -138,18 +154,17 @@ func runMachineAccountLogin(ctx context.Context, credentialsPath, hostname, apiH
138154
},
139155
}
140156

141-
// Use client_email as the keyring key when available; fall back to client_id.
142-
userKey := creds.ClientEmail
143-
if userKey == "" {
144-
userKey = creds.ClientID
145-
}
146-
147157
credsJSON, err := json.Marshal(stored)
148158
if err != nil {
149159
return fmt.Errorf("failed to serialize credentials: %w", err)
150160
}
151161

152162
if err := keyring.Set(authutil.ServiceName, userKey, string(credsJSON)); err != nil {
163+
// The PEM key was written to disk but the keyring write failed. Remove the
164+
// key file as best-effort cleanup so we don't leave crypto material behind.
165+
if cleanupErr := authutil.RemoveMachineAccountKeyFile(userKey); cleanupErr != nil {
166+
fmt.Printf("Warning: failed to remove machine account key file after keyring error for %s: %v\n", userKey, cleanupErr)
167+
}
153168
return fmt.Errorf("failed to store credentials in keyring for %s: %w", userKey, err)
154169
}
155170

0 commit comments

Comments
 (0)