Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(daemon): remove untrusted socket #361

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

thp-canonical
Copy link
Contributor

@thp-canonical thp-canonical commented Feb 15, 2024

As part of working on #358, we found out that the untrusted socket is not used, and so can be removed (in preparation for porting the AccessChecker changes from snapd in #358).

Indicators that it's not used:

If we look at how canAccess works, if we match on untrustedSocketPath (isUntrusted), the only way for canAccess to allow the request is when c.UntrustedOK is true (otherwise it unconditionally returns accessUnauthorized immediately):

if isUntrusted {
	if c.UntrustedOK {
		return accessOK
	}
	return accessUnauthorized
}

So in order for any API calls to be allowed with the untrusted socket (assuming all API calls go through canAccess), we would need to have a Command defined with UntrustedOK: true. Checking the Pebble codebase, no such Command definition exists, which means that even if any application would use the untrusted socket currently, all API calls would return accessUnauthorized unconditionally for this socket.

The untrusted socket as well as UntrustedOK in Command were already part of the initial import commit (50466ba), so seem to be an inheritance from snapd that haven't seen use in Pebble since then. The corresponding snapd sources from around November 10th, 2020 seem to call these SnapOK (UntrustedOK), dirs.SnapSocket (untrustedSocketPath) and snapListener (untrustedListener).

Due to gofmt and removal of struct members with the longest names, this PR is best reviewed with the "hide whitespace" option.

Copy link
Member

@hpidcock hpidcock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am happy with the justification for this removal. If at a point in the future we need such a socket, we can reintroduce it. Thoughts @benhoyt ?

@benhoyt
Copy link
Contributor

benhoyt commented Feb 16, 2024

Sorry, I've run out of time today -- will need to take a closer look early next week. (Sounds reasonable though.)

@benhoyt benhoyt changed the title chore(daemon): Remove untrusted socket chore(daemon): remove untrusted socket Feb 19, 2024
Copy link
Contributor

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I agree this removal/simplification makes sense -- in preparation for porting the AccessChecker changes from snapd in #358.

@benhoyt benhoyt closed this Feb 19, 2024
@benhoyt
Copy link
Contributor

benhoyt commented Feb 19, 2024

Oops, accidental close.

@benhoyt benhoyt reopened this Feb 19, 2024
@benhoyt benhoyt merged commit 7a139ff into canonical:master Feb 19, 2024
27 checks passed
@thp-canonical thp-canonical deleted the remove-untrusted branch February 19, 2024 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants