fix(alloc/exec): trim spaces on secretID to prevent error on lookup

[Thunderbottom]

If the ACL Token set in localstorage contains prefixed/suffixed spaces, any RPC calls to Allocations.Exec results in:

  Connection Closed: 1011 acl token lookup failed: index error: UUID must be 36 characters

How to reproduce:

1. Set token in https://<nomad-instance>/ui/settings/tokens, preferably with an added space before the token. The token gets set correctly, which means that the system is handling spaces just fine.

2. Exec into any allocation through the web UI, which would fail as described above.

Sample Request Payload:

{
  "version": 1,
  "auth_token": " XXXXXXXX-XXXX-XXXX-XXXX-XXXXXXXXXXXX",
}

Notice the prefixed added space in the auth_token value which causes the error.

Proposed fix:

RPC to Allocations.Exec calls ACLTokenBySecretID(), which should pass the space-trimmed secretID to lookup the ACL token. A simple strings.TrimSpace() call around secretID should fix the issue.

[tgross]

Hi @Thunderbottom! Thanks for opening this PR!

I think it's arguable whether or not we should be doing this in the API but this should definitely not be touched at the state store level. We should be doing any input validation "further out". Can you rework this so that it's happening when we initially validate the request?

[Thunderbottom]

I think it's arguable whether or not we should be doing this in the API but this should definitely not be touched at the state store level.

Hm, I believe prefixed and suffixed spaces have very little to contribute anyways to the token and can be stripped at store level validations. Could you let me know of any specific concerns that would make you vary of making such changes at this level?

Can you rework this so that it's happening when we initially validate the request?

Do you mean at the UI front or the backend? The current implementation handles all validation cases regardless where there's spaces in the token. I'd be more than happy to rework this if you point me to where I should be making changes. Sorry if that's asking for too much from you, and thanks for your time :)

[tgross]

Could you let me know of any specific concerns that would make you vary of making such changes at this level?

Primarily separation of concerns and long-term maintainability. Keeping validation code together makes sure we don't have parts of the code base that are handling unvalidated code when they shouldn't be.

Do you mean at the UI front or the backend? The current implementation handles all validation cases regardless where there's spaces in the token. I'd be more than happy to rework this if you point me to where I should be making changes. Sorry if that's asking for too much from you, and thanks for your time :)

Where we parse the token from the header in the HTTP server seems like the right place for this kind of thing, I think? See http.go#L744-L750. That way every endpoint enjoys the fix and not just alloc/exec, right?

[hashicorp-cla]

All committers have signed the CLA.

[tgross]

It's been a bit since we've heard back on this, so going to close this in lieu of #16469. But thanks for bringing this to our attention!