Conversation
ayush-panta
force-pushed
the
macos-cred-improvements
branch
2 times, most recently
from
c151049 to
7bee38b
Compare
| } | ||
|
|
||
| // Start credential server after VM is running (no-op on Windows) | ||
| execPath, err := os.Executable() |
There was a problem hiding this comment.
how this is no op in windows?
How login and logout is handled in windows.
There was a problem hiding this comment.
Windows does not use the finchhost credential helper. Though it runs login/logout on host, they still interface with ~/.finch/config.json. As the nerdctl configuration in VM is effectively untouched for Windows, meaning ~/.finch/config.json is still available in Windows, this still works as expected.
There was a problem hiding this comment.
Can we add this to the comment, its unintuitive.
There was a problem hiding this comment.
+1 can you clarify this in the comment, not clear to me
ayush-panta
changed the title
ayush-panta
force-pushed
the
macos-cred-improvements
branch
from
e502bb6 to
b23336c
Compare
ayush-panta
force-pushed
the
macos-cred-improvements
branch
3 times, most recently
from
2100694 to
368008b
Compare
Signed-off-by: ayush-panta <ayushkp@amazon.com>
ayush-panta
force-pushed
the
macos-cred-improvements
branch
from
368008b to
a066a58
Compare
sondavidb
left a comment
sondavidb
left a comment
There was a problem hiding this comment.
Mostly small changes LGTM overall
| logrus.WithField("socket", socketPath).Info("Starting credential server") | ||
|
|
||
| // Remove stale socket if it exists | ||
| _ = os.Remove(socketPath) |
There was a problem hiding this comment.
Should we not throw an error if there is one when removing?
| // Create listener and socket | ||
| listener, err := net.Listen("unix", socketPath) | ||
| if err != nil { | ||
| logrus.Fatalf("Failed to create socket: %v", err) |
There was a problem hiding this comment.
Does logrus failing auto-exit the log process and thus the whole program? If not, we should make sure to explicitly exit as well. (Applicable to a lot of the rest of this file too.)
| hostSocket: "{{.Dir}}/sock/finch.sock" | ||
| - guestSocket: "/run/user/{{.UID}}/finch-creds.sock" | ||
| hostSocket: "{{.Dir}}/sock/creds.sock" | ||
| reverse: true No newline at end of file |
| } | ||
| } | ||
|
|
||
| func credHelperDefault(cfg *Finch) { |
There was a problem hiding this comment.
Do we need to add the defaults for Windows?
| // The default location of DOCKER_CONFIG is ~/.docker/config.json. This config change sets the location to | ||
| // ~/.finch/config.json, but from the perspective of macOS (/Users/<user>/.finch/config.json). | ||
| // The default location of DOCKER_CONFIG is ~/.docker/config.json. This config change sets the location in a | ||
| // platform-specific manner. On macOS (Lima), this is set to ~/.finch/vm-config/config.json to allow nerdctl to |
There was a problem hiding this comment.
nit: Lima technically handles both Windows and Mac. Just that it defers to WSL on Windows.
| pid := strings.TrimSpace(string(data)) | ||
| if !isDaemonRunning(pidFile) { | ||
| logrus.Warnf("Stale PID file found (PID: %s), cleaning up", pid) | ||
| _ = os.Remove(pidFile) |
There was a problem hiding this comment.
Should we warn if this errors?
| cmd.Stdin = strings.NewReader(registryHostname) | ||
| output, err := cmd.CombinedOutput() | ||
| if err != nil { | ||
| // Do not include helper output in error as it may contain credentials |
There was a problem hiding this comment.
Shouldn't we avoid leaking credentials in errors anyway?
Might be good to have a sanitation function that comes w/ the credhelper funcs. (Fine as a followup)
3 participants
Description of changes: This PR contains an architectural rework for credential operations on macOS. This rework came out of necessity in the objective to remove plaintext credential storage for Finch by using the macOS keychain. The solution uses a custom credential helper in the VM in tandem with a credential process on the host to standardize how credentials are securely stored, erased, and retrieved.
Testing done: Unit tests have been included for the majority of added functions. Integration tests have been added under
test-e2e-vm-serialthat test workflows for credential operations with bothosxkeychainand the existing plaintext config. A bug bash has been conducted to confirm the implementation works across comprehensive user-scenarios.License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.