Capture ACL Token from self API call for Reverse Proxy use-case

[legege]

Proposed fix for issue #10561.

I looked into the source code and right now, the only places where a token secret is set are:

• nomad/ui/app/services/token.js

  Line 49 in 2766444

  this.secret = token.secret;

• nomad/ui/app/controllers/settings/tokens.js

  Line 43 in 2766444

  this.set('token.secret', secret);

We simply need to capture and store the secret in TokenService when call is made to the Token self API.

Here is a tentative implementation that I've tested here. I'm open for recommendations.

[backspace]

Hey, thanks for the pull request. I’ve been thinking about it and I have qualms to varying degrees, but there’s probably a way forward.

• this would erase an existing token if …/self didn’t include the token, which it doesn’t by default
  • easily addressed with a conditional, thankfully
• is it a best practice to couple the implementation to the token being injected in to the …/self response?
  • if so, it should be documented somewhere
• would another approach be for the reverse proxy to inject the header into UI requests and therefore not need to change the code at all?
  • it might be weird that you’d be operating a UI without an apparent token 🤔
• magic around tokens makes me wary of security holes, but I can’t actually think of any problems 🙃

Regardless of the implementation in the end, this is the kind of thing I’d like to see reflected in application tests, including one to test the conditional mentioned above 🤓

[legege]

Thanks @backspace for taking a closer look.

• is it a best practice to couple the implementation to the token being injected in to the …/self response?
  • if so, it should be documented somewhere

Let me provide an alternative implementation here: we can simply adjust the UI logic to always fetch self token even when secret is not set. I just tested this approach and it works. I think it's better code than catching the secret on the fly. What do you think? I did a spot check and there is no UI logic that depends on secret being set or not (other than the ACL Token page obviously, but that's ok).

   @computed('secret', 'fetchSelfToken.lastSuccessful.value')
   get selfToken() {
+    return this.get('fetchSelfToken.lastSuccessful.value');
-    if (this.secret) return this.get('fetchSelfToken.lastSuccessful.value');
-    return undefined;
   }

• would another approach be for the reverse proxy to inject the header into UI requests and therefore not need to change the code at all?
  • it might be weird that you’d be operating a UI without an apparent token 🤔

In fact, this is the idea here. The Reverse Proxy injecting the header into all requests (UI and API - but could be API only). With the alternative implementation described above, the UI would actually not have any apparent token (much like Consul UI does today when we use the same Reverse Proxy approach).

[legege]

@backspace I've updated the PR and added a test for this use-case. I simulated a reverse proxy by simply overriding XMLHttpRequest. Let me know what you think :-)

[tgross]

@picatz and @JBhagat841 I'd like to pull you both in for discussion on this one. It's not super clear to me what the security implications are.

[picatz]

Overall, I'm ok with the proposed changes from a security perspective. Allowing a reverse proxy in front of Nomad's HTTP server to inject the token header for the UI seems useful. 👍

The security implications are fairly nuanced, but I don't believe this would open up a security hole within Nomad itself. I can imagine issues with the reverse proxy implementation, but that would be outside the scope of Nomad.

FWIW, the original issue hinted at using SSO like Consul does today. If that's the intended use case here, it might be worth considering if that should be natively implemented instead of requiring a reverse proxy? Maybe that deserves its own issue / discussion, and I don't think would be a reason to block this PR.

[legege]

@tgross Is there anything that blocks this patch? I'm willing to help if needs be. Thanks

[lgfa29]

Hi @legege 👋

Sorry for the delay here, there's nothing blocking this PR from your side (the failed check is unrelated to your code change). We will review it shortly 👍

[lgfa29]

All looks good, thank you for the contribution @legege!

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.