Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Verify TLS certificate on endpoints that are used between agents only #11956

Merged
merged 9 commits into from
Feb 2, 2022

Conversation

lgfa29
Copy link
Contributor

@lgfa29 lgfa29 commented Jan 27, 2022

There are different types of endpoints in Nomad, and different types use different mechanism to validate and authenticate requests.

The most common endpoints are called by end-users via the HTTP API and get translated to an RPC request. They are authenticated using ACL tokens.

Another type of endpoint is used by Raft and are called between servers. This process doesn't have ACL tokens involved, so requests are validated by checking the mTLS certificates, when available.

Finally, there are endpoints that are called by agents directly via RPC, either client -> server or server -> server. These also don't use ACL tokens, so there was no explicit validation.

This PRs adds a check to these agent to agent endpoints similar to Raft's, using the mTLS certificate, if available.

nomad/alloc_endpoint.go Outdated Show resolved Hide resolved
nomad/server.go Show resolved Hide resolved
.changelog/11956.txt Outdated Show resolved Hide resolved
Copy link
Member

@shoenig shoenig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; just some quick thoughts

Copy link
Member

@schmichael schmichael left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Feel free to ship as-is.

nomad/rpc.go Outdated Show resolved Hide resolved
nomad/server.go Show resolved Hide resolved
nomad/server.go Show resolved Hide resolved
nomad/server.go Outdated Show resolved Hide resolved
@github-actions
Copy link

github-actions bot commented Feb 1, 2022

Ember Asset Size action

As of dd59dc5

Files that stayed the same size 🤷‍:

File raw gzip
nomad-ui.js 0 B 0 B
vendor.js 0 B 0 B
nomad-ui.css 0 B 0 B
vendor.css 0 B 0 B

@github-actions
Copy link

github-actions bot commented Feb 1, 2022

Ember Test Audit comparison

main dd59dc513feb76b7f179dda08113e5c7fe3ad4b5 change
passes 1270 1270 0
failures 0 0 0
flaky 0 0 0
duration 10m 39s 548ms 9m 53s 399ms -46s 149ms

@lgfa29
Copy link
Contributor Author

lgfa29 commented Feb 2, 2022

My merge to fix conflicts with main ended up messing some UI files...so I'm going to rebase and force push to undo them.

@vercel vercel bot temporarily deployed to Preview – nomad February 2, 2022 01:27 Inactive
@lgfa29 lgfa29 merged commit c613dc5 into main Feb 2, 2022
@lgfa29 lgfa29 deleted the f-verify-rpc-cert-origin branch February 2, 2022 20:03
lgfa29 added a commit that referenced this pull request Feb 3, 2022
PR #11956 implemented a new mTLS RPC check to validate the role of the
certificate used in the request, but further testing revealed two flaws:

  1. client-only endpoints did not accept server certificates so the
     request would fail when forwarded from one server to another.
  2. the certificate was being checked after the request was forwarded,
     so the check would happen over the server certificate, not the
     actual source.

This commit checks for the desired mTLS level, where the client level
accepts both, a server or a client certificate. It also validates the
cercertificate before the request is forwarded.
lgfa29 added a commit that referenced this pull request Feb 5, 2022
PR #11956 implemented a new mTLS RPC check to validate the role of the
certificate used in the request, but further testing revealed two flaws:

  1. client-only endpoints did not accept server certificates so the
     request would fail when forwarded from one server to another.
  2. the certificate was being checked after the request was forwarded,
     so the check would happen over the server certificate, not the
     actual source.

This commit checks for the desired mTLS level, where the client level
accepts both, a server or a client certificate. It also validates the
cercertificate before the request is forwarded.
@lgfa29 lgfa29 added backport/1.1.x backport to 1.1.x release line backport/1.2.x backport to 1.1.x release line labels Apr 19, 2022
@shoenig shoenig mentioned this pull request Jun 2, 2022
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/1.1.x backport to 1.1.x release line backport/1.2.x backport to 1.1.x release line
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants