feat(registry): enhance authentication checks in htpasswd implementation

[mnixry]

• supersedes, closes 4758 review suggestions #4830

Ref: goharbor/harbor-helm#2293

This PR improves error handling for the htpasswd authenticator. Previously, unsupported algorithms or malformed hashes triggered a generic 'authentication failure,' making it hard to debug configuration issues. The logs will now be more specific.

[milosgajdos]

PTAL @thaJeztah @wy65701436 @squizzi

[milosgajdos]

@mnixry mind rebasing? we've merged something that fixed Azure tests

[thaJeztah]

Thanks! I left some suggestions; let me also push my branch in which I made (these changes)

[thaJeztah]

The actual error returned from this function is discarded, and only used as "boolean" value and for logging;

	if err := localHTPasswd.authenticateUser(username, password); err != nil {
		dcontext.GetLogger(req.Context()).Errorf("error authenticating user %q: %v", username, err)
		return nil, &challenge{
			realm: ac.realm,
			err:   auth.ErrAuthenticationFailure,
		}
	}

Perhaps we should change the encapsulation, and handle logging here to make sure the returned error would never surface to the user (potentially opening similar attack vectors as mentioned above);

We can pass the context to authenticateUser and make the caller use the actual error returned, which can now safely be used;

	if err := localHTPasswd.authenticateUser(req.Context(), username, password); err != nil {
		return nil, &challenge{
			realm: ac.realm,
			err:   err,
		}
	}

BUT change the localHTPasswd.authenticateUser to handle both logging and to return the "safe to use", "safe to return" error.

// dummyBcryptHash is used for non-existing users to prevent timing attacks.
const dummyBcryptHash = "$2a$05$AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"

var dummyHash = []byte(dummyBcryptHash)

// AuthenticateUser checks a given user:password credential against the
// receiving HTPasswd's file. If the check passes, nil is returned.
func (htpasswd *htpasswd) authenticateUser(ctx context.Context, username string, password string) error {
	credentials, ok := htpasswd.entries[username]
	if !ok {
		// timing attack paranoia, always compare the hash even if the user is not found.
		_ = bcrypt.CompareHashAndPassword(dummyHash, []byte(password))
		return auth.ErrAuthenticationFailure
	}

	if err := bcrypt.CompareHashAndPassword(credentials, []byte(password)); err != nil {
		if !errors.Is(err, bcrypt.ErrMismatchedHashAndPassword) {
			dcontext.GetLogger(ctx).WithError(err).Errorf("error authenticating user %q", username)
		}
		// Always return a generic "ErrAuthenticationFailure", not the underlying error.
		return auth.ErrAuthenticationFailure
	}

	return nil
}

Doing so encapsulates the logic to this function, making it clear within scope what we're doing, and does not require special handling on the calling site.

[mnixry]

@thaJeztah Thanks for the suggestion! These changes look very neat to me overall.

I do have one small question, though: it seems this would change the behavior for failed authentication attempts. Previously, any authentication request for an existing user, including cases where authentication failed because of an incorrect password would be logged. With the current change, it looks like failed authentication would only be logged when the htpasswd file is misconfigured.

Is that behavior change intentional, and do we consider it acceptable?

[thaJeztah]

Good question; it was mostly intentional, because a user sending an invalid password is not an error in the registry; it's just an invalid request (and returned by the registry as an error to the user). At least it should not be logged as an .Error / .Errorf, and even Warn or Info is probably too much, but we could add a Debug log; maybe these 4xx errors are already logged elsewhere as debug logs (not sure)?

@milosgajdos any thoughts / opinions? (Debug, or at most Info seems suitable if we would add back something)

[milosgajdos]

I think I'd smash Info, because it could be useful to operators who the end users usually bug about things like this, but I think Debug works, too tbh

[thaJeztah]

• pushed as 4758 review suggestions #4830, but would need a squash / cleanup 😅

[thaJeztah]

Sorry for the duplicated comments; I think GitHub is kinda broken on reviews 😂

[milosgajdos]

@mnixry you still interested in getting this in?

[mnixry]

you still interested in getting this in?

@milosgajdos Thanks for the reminder. I do want to get this merged, but I’ve been a bit distracted by other work lately :(

I’ve now cherry-picked @thaJeztah’s suggested changes, which look very neat and make a lot of sense to me, and I also made a few minor fixes for testes that weren’t covered by the suggestion commit.

[milosgajdos]

PTAL @thaJeztah since you requested the changes

[thaJeztah]

Did a quick rebase and squash, and added "Info" logs for regular errors (i.e., user mis-typed password 😂)

[thaJeztah]

LGTM

@milosgajdos PTAL