Add apt credentials parser #60

woky · 2023-05-15T19:52:27Z

Add apt.auth.conf(5) machine credentials parser. See the parser code for
documentation.

This commit doesn't change the current behavior. It's only a preparation
for future commits that will add support for different types of archives.

This PR depends on #55.

Have you signed the CLA?

rebornplusplus

Looks nice! I only have a few queries.

internal/archive/credentials.go

Add apt.auth.conf(5) machine credentials parser. See the parser code for documentation. This commit doesn't change the current behavior. It's only a preparation for future commits that will add support for different types of archives.

niemeyer

Thanks Thomas, this is looking good. Here is a fisrt pass:

internal/archive/export_test.go

internal/archive/credentials_test.go

internal/archive/credentials.go

niemeyer · 2023-06-26T14:47:36Z

internal/archive/credentials.go

+	token := p.scanner.Text()
+	if i := strings.Index(token, "://"); i != -1 {
+		if token[0:i] != p.query.scheme {
+			return netrcInvalid, nil


So this gets back to the start state? Again, the idea of the parser being in an invalid state and just going on is a bit confusing, and it seems to relate to the bug mentioned earlier as it just ignores the current context while looking for special keywords. We'll probably need to sync on this with more bandwidth.

This is how the format is defined and parsed by Apt. It deliberately ignores current context. We're not at liberty to interpret the file differently.

internal/archive/credentials.go

niemeyer · 2023-06-26T14:53:54Z

internal/archive/credentials.go

+			if !strings.HasPrefix(token, p.query.port) {
+				return netrcInvalid, nil
+			}
+			token = token[len(p.query.port):]


This looks like it'll match the port ":90" when looking at ":9000".

The check below mitigates that

if !strings.HasPrefix(p.query.path, token) { return netrcInvalid, nil }

internal/archive/credentials.go

Remove global comment.

Test findCredsInDir() instead of findCredentials(). The latter calls the former with the directory set to a CHISEL_AUTH_DIR environment variable value if it's non-empty or "/etc/apt/auth.conf.d".

Adorn error return value.

Report filename on parser error.

Report file path on os.Open() error too.

Skip files with invalid extension before stat()-ing them.

internal/archive/credentials.go

cjdcordeiro · 2023-07-17T09:33:24Z

internal/archive/credentials.go

+	}
+	var err error
+	for state := netrcInvalid; state != nil; {
+		state, err = state(&p)


that logic is true @woky.

Some questions though:

if err != nil and state == nil, then do we need to continue to line 221? If not, better check the err here and just return

as you say, the above condition is by design and It would be a bug for a state to return both state and err non-nil.. So isn't that the check you should be doing? I.e. if state != nil && err != nil?

This reverts commit c6db9e7.

flotter

Looks really great. I did a review just to help out and improve my Go skills :)

flotter · 2023-07-21T15:33:20Z

internal/archive/credentials.go

+func findCredentialsInDir(repoURL string, credsDir string) (creds credentials, err error) {
+	contents, err := os.ReadDir(credsDir)
+	if err != nil {
+		if os.IsNotExist(err) {


Just an observation: here we are concluding that no credentials exist, which could be reached if CHISEL_AUTH_DIR is set, but pointing to a non-existing directory. This may be by design, but just pointing out that this does not mean that /etc/apt/auth.conf.d does not contain credentials.

It is indeed by my design. :-) We did not specify this, so I did what felt intuitive to me. Warning when CHISEL_AUTH_DIR doesn't exist and ignoring non-existent /etc/apt/auth.conf.d feels inconsistent to me. Would you like a different behavior?

Example from elsewhere: mpv looks up its configs in $MPV_HOME or in ~/.config/mpv/ (among other locations). It doesn't complain if either of those doesn't exist. Similarly, git doesn't complain when $GIT_CONFIG_SYSTEM or /etc/gitconfig doesn't exist but it reads it if it does. In both cases, if the variable is set (or non-empty?), the default location is skipped.

I see both of your points. I think it would help to simply have a debug log message to say that CHISEL_AUTH_DIR's path does not exist, regardless of its path.

It does make sense to me, the naming of the environment variable suggests an overriding behaviour, which if abused, must yield consequences :)

@cjdcordeiro @flotter I've added a debug message when the credentials directory does not exist, regardless of whether it's the default location or overridden by the variable. See commit 482ff15. Please resolve if you think it addresses your comment.

As a good rule of thumb, any function that is requested to perform a given task must either succeed at its task, or return an error informing that it could not. That's even more true when the function actually returns an error, as the caller will imply that unless there's an error, its intention was successful. The task of this function is to find credentials in a directory, so it must either succeed at that, or return an error informing that it cannot perform its requested task.

The call site, though, may choose to ignore a given scenario. For handling some of those cases we have error results that can be more easily verified, either directly or via a function.

Introduced ErrCredentialsNotFound error. The function now returns this error when no credentials are found. @niemeyer Please resolve the thread if appropriate.

internal/archive/credentials.go

flotter

Proposed changes looks great, thanks!

niemeyer

Some additional notes.

internal/archive/credentials.go

niemeyer · 2023-08-07T15:33:14Z

internal/archive/credentials.go

+func findCredentialsInDir(repoURL string, credsDir string) (creds credentials, err error) {
+	contents, err := os.ReadDir(credsDir)
+	if err != nil {
+		if os.IsNotExist(err) {


As a good rule of thumb, any function that is requested to perform a given task must either succeed at its task, or return an error informing that it could not. That's even more true when the function actually returns an error, as the caller will imply that unless there's an error, its intention was successful. The task of this function is to find credentials in a directory, so it must either succeed at that, or return an error informing that it cannot perform its requested task.

The call site, though, may choose to ignore a given scenario. For handling some of those cases we have error results that can be more easily verified, either directly or via a function.

niemeyer · 2023-08-07T15:34:47Z

internal/archive/credentials.go

+	}
+	if len(confFiles) == 0 {
+		err = errors.Join(errs...)
+		return


Similar to the earlier point on errors here: the loop above may skip all the way through, and we end up with no credentials and no error either.

As I commented above, I've introduced ErrCredentialsNotFound error. The function now returns this error when no credentials are found. @niemeyer Please resolve the thread if appropriate.

niemeyer · 2023-08-07T15:35:51Z

internal/archive/credentials.go

+// directory specified by CHISEL_AUTH_DIR environment variable if it's
+// non-empty or /etc/apt/auth.conf.d.
+func findCredentials(repoUrl string) (credentials, error) {
+	credsDir := "/etc/apt/auth.conf.d"


Per earlier comments, the default path should be a global variable, or a constant, according to your preference, and the logic must be tested according to these notes too. This function, and in particular the use of CHISEL_AUTH_DIR, looks untested?

niemeyer · 2023-08-07T15:39:43Z

internal/archive/credentials.go

+		err = fmt.Errorf("cannot parse archive URL: %w", err)
+		return
+	}
+	if query == nil { // creds.Empty() == false


This needs to be improved a bit so that the intention is more clear. If the point here is that we already have credentials, we should check creds explicitly instead of checking query and assuming credentials are set in a comment.

Replaced with !creds.Empty(). I can't rely on err != nil here as that alone does not tell me whether the URL contained credentials. @niemeyer Please resolve the thread if appropriate.

niemeyer · 2023-08-07T15:40:31Z

internal/archive/credentials.go

+		return
+	}
+
+	errs := make([]error, 0)


What's the intention here? Why is this not just var errs []error?

As part of the change to return ErrCredentialsNotFound when no credentials are found, non-fatal errors are now logged instead so the function no longer collects them, and this variable was removed. @niemeyer Please resolve the thread if appropriate.

internal/archive/credentials.go

niemeyer · 2023-08-07T15:49:32Z

internal/archive/credentials.go

+
+		if err = findCredsInFile(query, f, &creds); err != nil {
+			errs = append(errs, fmt.Errorf("cannot parse credentials file %s: %w", fpath, err))
+		} else if !creds.Empty() {


Let's please have a more clear interface on findCredsInFile, making it return the credentials when it finds them, as usual in Go, instead of having C-style output parameters unnecessarily. We should also not have to test creds for being empty. Per earlier notes, if the error is nil, the intent of the function must have been fulfilled.

As I commented above, I've introduced ErrCredentialsNotFound error. The function findCredsInFile was renamed to findCredentialsInternal and it now returns either a non-nil pointer to credentials or a non-nil error when an error occurs or credentials are not found. @niemeyer Please resolve the thread if appropriate.

niemeyer · 2023-08-07T15:50:39Z

internal/archive/credentials.go

+//	[3] https://salsa.debian.org/apt-team/apt/-/blob/4e04cbaf/methods/aptmethod.h#L560
+//	[4] https://www.gnu.org/software/inetutils/manual/html_node/The-_002enetrc-file.html
+//	[5] https://daniel.haxx.se/blog/2022/05/31/netrc-pains/
+func findCredsInFile(query *credentialsQuery, netrc io.Reader, creds *credentials) error {


Per note above, which of these is input and which of these is output? Let's please clean up the function prototype to reflect that, and have the error result reflect intent, per notes above.

Commented about this function in the above thread. @niemeyer Please resolve the thread if appropriate.

niemeyer

Thanks for the implementatino and the tuning!

niemeyer · 2023-08-11T15:07:17Z

internal/archive/credentials.go

+//	[2] https://salsa.debian.org/apt-team/apt/-/blob/d9039b24/apt-pkg/contrib/netrc.cc
+//	[3] https://salsa.debian.org/apt-team/apt/-/blob/4e04cbaf/methods/aptmethod.h#L560
+//	[4] https://www.gnu.org/software/inetutils/manual/html_node/The-_002enetrc-file.html
+//	[5] https://daniel.haxx.se/blog/2022/05/31/netrc-pains/


While we're still here, thanks for the extensive documentation and references above.

woky requested review from cjdcordeiro, niemeyer and rebornplusplus May 15, 2023 19:52

woky mentioned this pull request May 15, 2023

Add support for Pro FIPS archives #61

Closed

1 task

woky force-pushed the pro-new/netrc-parser branch 2 times, most recently from e38fe87 to f5faec1 Compare May 15, 2023 21:22

woky mentioned this pull request May 15, 2023

Add support for pro archives #53

Closed

woky force-pushed the pro-new/netrc-parser branch 3 times, most recently from caabf58 to 41e2547 Compare May 17, 2023 08:47

woky assigned niemeyer May 17, 2023

woky force-pushed the pro-new/netrc-parser branch 3 times, most recently from 3d20fa5 to e708109 Compare May 18, 2023 21:23

cjdcordeiro added the Priority Look at me first label May 22, 2023

woky force-pushed the pro-new/netrc-parser branch 3 times, most recently from 7a08f21 to f9e2c5c Compare May 24, 2023 20:12

rebornplusplus reviewed May 29, 2023

View reviewed changes

internal/archive/credentials.go Outdated Show resolved Hide resolved

internal/archive/credentials.go Outdated Show resolved Hide resolved

internal/archive/credentials.go Outdated Show resolved Hide resolved

cjdcordeiro reviewed May 30, 2023

View reviewed changes

internal/archive/credentials.go Outdated Show resolved Hide resolved

internal/archive/credentials.go Show resolved Hide resolved

woky force-pushed the pro-new/netrc-parser branch 2 times, most recently from 64391b9 to 1babeb0 Compare June 21, 2023 18:05

Add apt credentials parser

f573944

Add apt.auth.conf(5) machine credentials parser. See the parser code for documentation. This commit doesn't change the current behavior. It's only a preparation for future commits that will add support for different types of archives.

woky force-pushed the pro-new/netrc-parser branch from 1babeb0 to f573944 Compare June 22, 2023 04:28

cjdcordeiro requested a review from rebornplusplus June 22, 2023 12:42

niemeyer requested changes Jun 26, 2023

View reviewed changes

niemeyer added Reviewed Supposedly ready for tuning or merging and removed Priority Look at me first labels Jun 26, 2023

woky added 2 commits June 28, 2023 13:53

fixup! Add apt credentials parser

88c1fb0

Remove global comment.

fixup! Add apt credentials parser

cfcc048

Test findCredsInDir() instead of findCredentials(). The latter calls the former with the directory set to a CHISEL_AUTH_DIR environment variable value if it's non-empty or "/etc/apt/auth.conf.d".

woky added 2 commits June 28, 2023 14:18

fixup! Add apt credentials parser

8be5a54

Adorn error return value.

fixup! Add apt credentials parser

518af57

Report filename on parser error.

woky added Priority Look at me first and removed Reviewed Supposedly ready for tuning or merging labels Jun 28, 2023

woky requested review from niemeyer and cjdcordeiro June 28, 2023 12:36

woky added 3 commits June 28, 2023 14:43

fixup! Add apt credentials parser

8c109e3

Report file path on os.Open() error too.

fixup! Add apt credentials parser

959c8c2

Skip files with invalid extension before stat()-ing them.

fixup! fixup! Add apt credentials parser

c6db9e7

cjdcordeiro reviewed Jul 17, 2023

View reviewed changes

woky added 2 commits July 17, 2023 13:29

Revert "fixup! fixup! Add apt credentials parser"

552b996

This reverts commit c6db9e7.

fixup! Add apt credentials parser

dc4f7d9

cjdcordeiro requested a review from flotter July 17, 2023 16:05

fixup! Add apt credentials parser

715a51c

flotter approved these changes Jul 23, 2023

View reviewed changes

woky added 4 commits July 24, 2023 19:10

fixup! Add apt credentials parser

5f4627f

fixup! Add apt credentials parser

b50901e

fixup! Add apt credentials parser

010e969

fixup! Add apt credentials parser

5e335b1

woky requested a review from flotter July 25, 2023 14:38

fixup! Add apt credentials parser

482ff15

flotter approved these changes Jul 25, 2023

View reviewed changes

niemeyer requested changes Aug 7, 2023

View reviewed changes

fixup! Add apt credentials parser

db609e1

woky requested review from niemeyer and cjdcordeiro August 8, 2023 12:19

fixup! Add apt credentials parser

ddefc45

woky force-pushed the pro-new/netrc-parser branch from 1bcbb91 to ddefc45 Compare August 9, 2023 19:56

niemeyer approved these changes Aug 11, 2023

View reviewed changes

niemeyer merged commit 86bcf61 into canonical:main Aug 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add apt credentials parser #60

Add apt credentials parser #60

woky commented May 15, 2023 •

edited

Loading

rebornplusplus left a comment

niemeyer left a comment

niemeyer Jun 26, 2023

woky Jun 28, 2023 •

edited

Loading

niemeyer Jun 26, 2023

woky Jun 28, 2023

cjdcordeiro Jul 17, 2023

flotter left a comment

flotter Jul 21, 2023

woky Jul 24, 2023 •

edited

Loading

cjdcordeiro Jul 25, 2023

flotter Jul 25, 2023

woky Jul 25, 2023

niemeyer Aug 7, 2023

woky Aug 8, 2023 •

edited

Loading

flotter left a comment

niemeyer left a comment

niemeyer Aug 7, 2023

niemeyer Aug 7, 2023

woky Aug 8, 2023 •

edited

Loading

niemeyer Aug 7, 2023

niemeyer Aug 7, 2023

woky Aug 8, 2023 •

edited

Loading

niemeyer Aug 7, 2023

woky Aug 8, 2023 •

edited

Loading

niemeyer Aug 7, 2023

woky Aug 8, 2023 •

edited

Loading

niemeyer Aug 7, 2023

woky Aug 8, 2023

niemeyer left a comment

niemeyer Aug 11, 2023

Add apt credentials parser #60

Add apt credentials parser #60

Conversation

woky commented May 15, 2023 • edited Loading

rebornplusplus left a comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flotter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Jul 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

flotter left a comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky Aug 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woky commented May 15, 2023 •

edited

Loading

woky Jun 28, 2023 •

edited

Loading

woky Jul 24, 2023 •

edited

Loading

woky Aug 8, 2023 •

edited

Loading

woky Aug 8, 2023 •

edited

Loading

woky Aug 8, 2023 •

edited

Loading

woky Aug 8, 2023 •

edited

Loading

woky Aug 8, 2023 •

edited

Loading