No nonce signature parameter fails validation

[mariusor]

Hello, I'm having issues with validation of requests that don't contain "nonces", which fail with the message of "nonce already seen".

This happens for the cases where there is no "nonce" in the signature parameters, not when the nonce is actually empty.

Because in the calling code I can't know if the nonce is present in the signature-input header (unless I implement my own unmarshaling for it) to be able to skip passing a nonce checker to the verifier, shouldn't only call the validation when noncePresent == true?

httpsig/verifier.go

Lines 295 to 302 in 3704855

	nonceValue, noncePresent := params.Params.Get(string(Nonce))
	if noncePresent {
	nonce = nonceValue.(string) //nolint: forcetypeassert
	}
	if err := nc.CheckNonce(msg.Context, nonce); err != nil {
	return fmt.Errorf("%w: nonce validation failed: %w", ErrParameter, err)
	}

So, I guess my question is: should I handle the case for no "nonce" signature parameter in the calling code, or do you consider this to be an actual bug?

[dadrus]

Hi. Thank you for that finding. Yes, it is a bug. There is obviously no need to check the nonce, if it is not present. If you like create a bug fix PR for that. The if clause in lines 300-302 need simply be moved into the if noncePresent clause.

[dadrus]

Just out of interest: How do you deal with nonces? This lib was implemented out of a need to support HTTP Message Signatures (HMS) in my other project (https://github.com/dadrus/heimdall). I don't make use of nonces with HMS in it now, but will need that in the near future, as soon as I continue with (dadrus/heimdall#2819). However, I have another feature I'm working on (dadrus/heimdall#3231), where I need nonces. You can find my current design/implementation for that here: https://github.com/dadrus/heimdall/blob/feat/dpop_support/internal/rules/mechanisms/nonce/nonce.go. It might be an overkill and there might be more performant ways to achieve the same level of security.

[mariusor]

So, I've created a PR to move the check: #106

I haven't really delved too deeply into how to deal with the nonces, I'm using the crypto rand.Text() for generating them, and an in memory store using a very shallow wrapper around a sync.Map.

In the corner of the internet where I'm using this (the ActivityPub based "fediverse") RFC9421 HTTP-Signatures are barely starting to show up, and nobody really bothers with nonces until now (based on production data).

Use of signer: https://github.com/go-ap/client/blob/master/s2s/httpsignatures.go#L230-L249
Nonce generator: https://github.com/go-ap/client/blob/master/s2s/httpsignatures.go#L140
Nonce checker making use of a static variable: https://github.com/go-ap/auth/blob/master/rfc-verifier.go#L13-L30
Verifier: https://github.com/go-ap/auth/blob/master/rfc-verifier.go#L48-L63

I'm not sure this answers your question exactly, so let me know if you want to know something else.

[dadrus]

Ah... wait. How to know whether the nonce is expected? That was the reason, why the CheckNonce is always called. The lib by itself is stateless. Only the consumer side can know whether the nonce is expected or not, and based on that it configures the lib accordingly. Given your description, it seems, you need to differentiate between an empty nonce and no nonce.

I newer thought that differentiation would be needed. The initial idea was that if the consumer side sets the nonce source and the nonce checker, the corresponding implementation can manage, which HMS must have a nonce and which not. And since in my opinion, an empty nonce is the same as no nonce, the NonceChecker interface was designed the way it is. I though, the context.Context object would be enough to make the consuming side being able to differentiate between different situations, where the nonce is required and where not. I mean, this is the consuming side, which also sets the expected labels, tag and other options. This way it should also be possible to attach the required information to the context object and then extract it again in the corresponding NonceGetter and NonceChecker implementations.

Obviosly, that design does not work for you. Or it does, and you have not considered that option?

Btw. if you like join heimdalls discord: https://discord.gg/qQgg8xKuyb. It might be easier to discuss this topic face to face.

[dadrus]

What I actually wanted to say is that my suggestion in #105 (comment) was a bad idea. It would result in a security issue if the application expects the nonce, but since it is not present, it would not be checked and assumed, the HMS is valid.

If the context based passing of information as described in my previous comment does not work the way, I was thinking it would work, there is a need to extend the NonceGetter and NonceChecker interfaces with additional arguments, which would pass the context of the HMS, like e.g. the tag, the label, or both to give the lib consumer implementation the ability to differentiate between different HMS configurations. And if there still the need to signal whether the nonce was present, but empty - which I still think is a bad design - the NonceChecke would also get an additional bool argument.

What do you think?

[dadrus]

Btw. thank you for sharing the code. I took a look at it. Not sure, whether you add some information to the request context in both cases, but obviously, you're not using it in both, the NonceGetter and NonceChecker implementations.

I have some questions related to your nonce usage, but will ask them later after we've resolved the actual topic :)

[mariusor]

I think just checking if nonceChecker was set could provide a calling code method to let the library know if the nonce needs to be checked or not.

httpsig/verifier.go

Line 296 in 3704855

if noncePresent {

So the code becomes something like this:

if noncePresent {
	nonce = nonceValue.(string) //nolint: forcetypeassert
}
if nonceCheckWanted := nc != nil; nonceCheckWanted {
	if err := nc.CheckNonce(msg.Context, nonce); err != nil {
		return fmt.Errorf("%w: nonce validation failed: %w", ErrParameter, err)
	}
}

This is of-course conditional on initializing the lib with a nil checker.

[dadrus]

This is not required. The noop nonce checker is configured by default:

httpsig/verifier.go

Lines 198 to 202 in 3704855

	ver := &verifier{
	keyResolver: resolver,
	tagExpectations: make(map[string]*expectations),
	nonceChecker: noopNonceChecker{},
	}

Similar defaults are also set for the signer:

httpsig/signer.go

Lines 109 to 115 in 3704855

	sig := &signer{
	label: "sig",
	ttl: 30 * time.Second, //nolint:mnd
	ng: nonceGetter{},
	ps: ps,
	mu: noopMessageUpdater{},
	}

[dadrus]

I fear, it does not solve your challenge. Correct?

[mariusor]

It does not, I think I'm looking for a hybrid logic:

1. If there's a nonce parameter, I want it to be checked.
2. If there isn't a nonce parameter, I want it to not be checked.

Currently the workaround I'm using is to special case empty string in my nonce checker: https://github.com/go-ap/auth/blob/master/rfc-verifier.go#L20

To me the fact that a nonce checker is provided does not mean that if the nonce parameter is missing, the signature is invalid. Most of the time, or at least in my case, there's no control over the signatures that can arrive, all I can do is to provide a sane set of defaults that would work for most of the cases.

[mariusor]

I also think that having the nonce checker be a interface instead of a plain function call is a bit of a problem with regards to the nil check above. (I'm thinking of the case where the interface value is not nil, but the underlying checker struct pointer is)

I think passing a type NonceSeenFn func(context.Context, string) bool/error would simplify this logic to some extent, and it wouldn't prevent implementers to pass a method from a more complicated type as a value". However that would be a breaking API change I think, and probably out of the question. :D

[dadrus]

To me the fact that a nonce checker is provided does not mean that if the nonce parameter is missing, the signature is invalid. Most of the time, or at least in my case, there's no control over the signatures that can arrive, all I can do is to provide a sane set of defaults that would work for most of the cases.

I see. But it is particular semantic of your setup and I fear it cannot be generalized in the lib itself, at least I don't know how.

I also think that having the nonce checker be a interface instead of a plain function call is a bit of a problem with regards to the nil check above. (I'm thinking of the case where the interface value is not nil, but the underlying checker struct pointer is)

Well, the current implementation does not do this nil check, as there is a noop nonce checker by default. And if the consumer of the lib passes not nil interface with a nil underlying struct, it is actually a bug on the consumer side, which would result in a panic - wrong usage of the lib. But we can definitely have an adapter function, like

type CheckNonceFn func(ctx context.Context, nonce string) error

func (f CheckNonceFn) CheckNonce(ctx context.Context, nonce string) error {
  return f(ctx, nonce) error
}

to ease usage.

Anyways, let me try to summarize our discussion:

• Since the lib doesn't have any idea of the semantics of the nonce, it is the responsibility of the consumer side to do the proper verification.
• The lib extracts the nonce - but there is indeed no way for the consumer to understand whether the nonce was present, but empty, or there was no nonce. In both cases the consumer will get an empty string
• The lib implementation assumes that if the nonce checker was configured, the consumer wants the nonce to be present. This is not the desired behavior semantics in your case
• I would indeed like avoiding breaking changes, but it doesn't mean, there shouldn't be any.

The only way to resolve the point 2 and 3 would be a breaking change - either adding an additional bool argument, or by changing the existing nonce type from string to a string pointer. I'm ok with both. Though, would prefer a string pointer to avoid blowing up the arguments list.

And since there will be a breaking change to resolve the bug, we can also talk about the alternative implementation:

I think passing a type NonceSeenFn func(context.Context, string) bool/error would simplify this logic to some extent, and it wouldn't prevent implementers to pass a method from a more complicated type as a value". However that would be a breaking API change I think, and probably out of the question. :D

There are two things, you're suggesting:

• instead of a neutral check - which can be anything - you're suggesting (given the name) an alternative semantics, which is actually not neutral any more. Not sure, whether we should do so.
• using a function instead of an interface. On the one hand, I'm with you, it would eliminate the wrapped nil. But is it an issue at all? There is and will be anyway a noop implementation if the corresponding funtion is not configured. If the concern is easier passing a function instead of having the need to implement the interface, what about an adapter function, as shown above? And if the concern is a nil wrapped in an interface - well, you know my opinion - it's a bug on the consumer side. Tough, we could add some reflection magic to the corresponding option to check for such situations in addition to the regular nil check

[mariusor]

First of all, I have to say that I'm also fine with leaving things as they are. The special casing on empty string on the checker end is easy enough to keep track of.

However I'm not convinced - and nothing I've read in the RFC makes me believe - that if a nonce is not present in the signature, verification should fail, even in the case where a nonce verification method exists. I hope that you agree with me that when the same nonce is received multiple times, it signifies a request replay attack, but if no nonce is present in the request, it signifies nothing from this point of view.

To my interpretation, the current behaviour of the library is not in sync to the specification.

But, you're right, the library is meant to be used perhaps with a different intention than the spec allows for, and I'm fine with that.

But, I have maybe another proposal, and which feels to have a better backwards compatiblity with the current API.

There could be another nonce related option function which would work similarly to the WithCreatedTimestampRequired() and WithUpdatedTimestampRequired() functions.

The WithNonceRequired() would turn a flag on to fail on missing nonce, and when that is off it would skip the check.

Does that sound like a decent compromise?