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

Client certificate support #2956

Open
wants to merge 14 commits into
base: develop
Choose a base branch
from

Conversation

wrouesnel
Copy link
Contributor

@wrouesnel wrouesnel commented May 28, 2023

This PR adds client-certificate support to nginx-proxy-manager. Closes #768. Relates to #622.

A new SSL certificate is defined - "client certificate authority" - which allows uploading client CA certificates. These can then be assigned to Access Lists via the UI or API, and finally the Access List assigned to a host, which will thus enable Client Certificate Authorization for mutual TLS connections to the host.

This includes a slight revamp of the access-list system to implement client IP checks as geo directives. This allows the "Drop Unauthorized" function to simply not respond to clients from the wrong IP address, as well as allowing "Satisfy All" and "Satisfy Any" to include Client CA functionality - namely, using Satisfy Any is it possible to selectively require client certificates from some networks but not others (in my household the primary use-case of this is for Home Assistant to require certificates from the internet but not the local network).

image

image

image

image

Known Issues

  • Upgrading with the new access-list code will lead to broken proxies because the new ACL files aren't initially generated. Should this be handled in the migration script?

The default nginx 444 response drops the inbound connection without
sending any response to the client.
OpenSSL data parsing could be confused when parsing certificates which
have Country/Org and other parameters in the subject line.

This is fixed by writing a more robust parser of the output lines, and
using that to do parsing which now correctly handles this case.
Add initial support for managing Client Certificate Authority public
certificates as certificate objects in the database. The new provider
type 'clientca' is defined to implement this.
The frontend is modified to filter certificates from selector lists
so only non-clientca certificate types can be set as server certificates.
@wrouesnel wrouesnel changed the title [WIP] Client certificate support Client certificate support May 29, 2023
Client certificate support is added as a new separate type of option for
access-lists.

This commit is the support code to enable access-lists to contain
Client Certificate references.
@jc21
Copy link
Member

jc21 commented May 29, 2023

FYI the CI build is failing because the API returns new fields that are not defined in the Swagger/OpenAPI document:

backend/doc/api.swagger.json
[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  | [validateSwaggerSchema DEBUG] Endpoint: /nginx/proxy-hosts

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  | [validateSwaggerSchema DEBUG] Response Schema: {

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "id": 1,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "created_on": "2023-05-29 04:51:09",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "modified_on": "2023-05-29 04:51:09",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "owner_user_id": 1,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "domain_names": [

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "test.example.com"

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   ],

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "forward_host": "1.1.1.1",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "forward_port": 80,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "access_list_id": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "certificate_id": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "ssl_forced": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "caching_enabled": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "block_exploits": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "advanced_config": "",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "meta": {

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "letsencrypt_agree": false,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "dns_challenge": false

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   },

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "allow_websocket_upgrade": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "http2_support": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "forward_scheme": "http",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "enabled": 1,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "locations": [],

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "hsts_enabled": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "hsts_subdomains": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "drop_unauthorized": false,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "certificate": null,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "owner": {

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "id": 1,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "created_on": "2023-05-29 04:50:35",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "modified_on": "2023-05-29 04:50:35",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "is_deleted": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "is_disabled": 0,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "email": "admin@example.com",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "name": "Administrator",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "nickname": "Admin",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "avatar": "",

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     "roles": [

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |       "admin"

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     ]

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   },

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "access_list": null,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "use_default_location": true,

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   "ipv6": false

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  | }

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  | [validateSwaggerSchema ERROR] data should NOT have additional properties

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |     1) Should be able to create a http host

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  | 

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  | 

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   0 passing (676ms)

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   1 failing

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  | 

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |   1) Hosts endpoints

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |        Should be able to create a http host:

[2023-05-29T04:51:09.852Z] npm_pr-2956_7-cypress-sqlite-1  |      AssertionError: expected 'data should NOT have additional properties' to equal null

@jc21
Copy link
Member

jc21 commented May 29, 2023

...
[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |   "hsts_enabled": 0,

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |   "hsts_subdomains": 0,

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |   "drop_unauthorized": false,

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |   "certificate": null,

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |   "owner": {

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     "id": 1,

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     "created_on": "2023-05-29 05:03:42",

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     "modified_on": "2023-05-29 05:03:42",

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     "is_deleted": 0,

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     "is_disabled": 0,

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     "email": "admin@example.com",

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     "name": "Administrator",

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     "nickname": "Admin",

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     "avatar": "",

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     "roles": [

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |       "admin"

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |     ]

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |   },

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |   "access_list": null,

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |   "use_default_location": true,

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  |   "ipv6": false

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  | }

[2023-05-29T05:04:47.969Z] npm_pr-2956_8-cypress-sqlite-1  | [validateSwaggerSchema ERROR] data.drop_unauthorized should be integer

[2023-05-29T05:04:48.223Z] npm_pr-2956_8-cypress-sqlite-1  |     1) Should be able to create a http host

[2023-05-29T05:04:48.223Z] npm_pr-2956_8-cypress-sqlite-1  | 

[2023-05-29T05:04:48.223Z] npm_pr-2956_8-cypress-sqlite-1  | 

[2023-05-29T05:04:48.223Z] npm_pr-2956_8-cypress-sqlite-1  |   0 passing (639ms)

[2023-05-29T05:04:48.223Z] npm_pr-2956_8-cypress-sqlite-1  |   1 failing

[2023-05-29T05:04:48.223Z] npm_pr-2956_8-cypress-sqlite-1  | 

[2023-05-29T05:04:48.223Z] npm_pr-2956_8-cypress-sqlite-1  |   1) Hosts endpoints

[2023-05-29T05:04:48.223Z] npm_pr-2956_8-cypress-sqlite-1  |        Should be able to create a http host:

[2023-05-29T05:04:48.223Z] npm_pr-2956_8-cypress-sqlite-1  |      AssertionError: expected 'data.drop_unauthorized should be integer' to equal null

@wrouesnel wrouesnel force-pushed the client_certificate_support branch 2 times, most recently from 0aa73d6 to 1558f99 Compare May 29, 2023 13:59
@wrouesnel wrouesnel marked this pull request as draft May 29, 2023 14:02
This commit adds the basic support necessary to produce the combined
client CA files when certificates are updated.
When an access list contains client CAs, the combined CA auth file is
added to all location blocks via an `if` statement. This allows
LetsEncrypt and other support paths to work, while correctly denying
access to the protected resources.
drop_unauthorized returns 444 when a client is not authorized as opposed
to 403. It can be used with Client Certificate authorization.
@wrouesnel wrouesnel marked this pull request as ready for review May 30, 2023 22:46
@wrouesnel wrouesnel marked this pull request as draft May 30, 2023 22:46
@wrouesnel wrouesnel marked this pull request as ready for review May 31, 2023 02:56
This commit changes access-list IP directives to be implemented using
the nginx "geo" directive.

This allows IP-based blocks to return 444 (drop connection) on
authorization failure when the "Drop Unauthorized" is enabled.

It also allows the implementation of "Satisfy Any" with the new
client CA certificate support - i.e. Satisfy Any can allow clients
from the local network to skip client certificate challenge, or drop
down to requesting basic authentication.

It should be noted that including basic authentication requirements
in Satisfy Any mode does prevent a 444 response from being sent, as
the basic auth challenge requires the server to respond.
LibreSSL uses a different output separated and semantics, which broke
the X509 parser. With some slight modifications both can be supported.
@Lucifer1903
Copy link

I keep getting 400 Bad Request: The SSL certificate error.

Is there anyway I can turn on debugging or access Nginx config settings? I read that changing the
ssl_verify_depth might help. Even if it doesn't, being able to enable debugging with definitely help as there's not much information in the current logs.

@Gyarbij
Copy link

Gyarbij commented Aug 3, 2023

Any further progress on this?

@cammurray
Copy link

This is awesome - any commitment to pulling this PR in to master?

This request #69 relates to this - and it's been around since 2019!

@wrouesnel
Copy link
Contributor Author

This needs more testing at the moment. I've successfully used it with Home Assistant which is what inspired me, but I believe there's an issue with Auth All vs Auth Any (any will auth no one).

@cammurray
Copy link

@wrouesnel any start is a good start - let me get testing :)

@cammurray
Copy link

@wrouesnel this is working pretty well for me, but I have had to specify "Satisfy Any" in the access list, i'm not aware of what the implications are of this, however? By using the ACL with satify any, requiring client certificate, on any of my hosts - clients that have a cert are able to login, clients that don't have a cert are given an unauthorised straight away.

If "Satisfy Any" is not specified, the cert picker comes up, but it doesn't allow you to specify any of the certs..

@Lucifer1903
Copy link

I keep getting 400 Bad Request: The SSL certificate error.

Is there anyway I can turn on debugging or access Nginx config settings? I read that changing the ssl_verify_depth might help. Even if it doesn't, being able to enable debugging with definitely help as there's not much information in the current logs.

This issue was my fault. I believe I wasn't creating the certificates correctly.

@jc21 jc21 added the requires-verification Waiting for one or more people to confirm the fix label Aug 31, 2023
@wrouesnel
Copy link
Contributor Author

Okay did some more tests - the only access list issue is that if you have "Satisfy All" set, and no IPs in the Access List, then it's treated as a default failure which is not the original behavior. Should be easy enough to fix.

IP access list control was implemented as default success for an
empty access control list - but this had the effect of an empty list
default allowing if "Satisfy Any" was set.

Fortunately this was bugged, so empty lists default failed - but this
broke empty lists for "Satisfy All".

This patch is the correct fix: lists now always default fail, but an
empty list removes the check from access control considerations.

This restores the original implementations behavior and fixes the bug.
@wrouesnel
Copy link
Contributor Author

wrouesnel commented Aug 31, 2023

And fixed! It was one potentially serious bug ("Satisfy Any" and an empty IP list led to a default allow) which I never actually found because the template I wrote was bugged.

This has been fixed so IP lists now always default to fail, and an empty IP control list simple removes the check from being parsed in the host (which is the correct solution).

This removes the regression in behavior so I'd be content saying this is now finished.

@nginxproxymanagerci
Copy link

Docker Image for build 16 is available on DockerHub as jc21/nginx-proxy-manager:github-pr-2956

Note: ensure you backup your NPM instance before testing this PR image! Especially if this PR contains database changes.

@heyitsmdr
Copy link

Is this feature planned to get merged? I would absolutely love this without having to use a hacky workaround!

@oziee
Copy link

oziee commented Nov 25, 2023

@wrouesnel Will, you legend !!

now this is what I was looking for to lock down the nginx to a device with a cert :)

but now that I have this up and running, I created ca,server and client x509 certs and installed them in the nginx and in the phone but all I get is 403 error

any special requirements for the x509 certs?? do you have a write up on creating the certs ?? I guess I am creating them wrong or using the wrong ones in the nginx prox manager client access cert

@wrouesnel
Copy link
Contributor Author

@oziee I'd need more information to know what's not working. I do know that you can wind up having this pop up when you don't do the Android certificate load just right (I think in some cases I had to convert the certificate, and then add a pin in order to get Android to pick it up).

@oziee
Copy link

oziee commented Nov 27, 2023

Oh android only.. I'm iOS

@cgoIT
Copy link

cgoIT commented Nov 28, 2023

It should also work on iOS. Maybe it's a little bit more difficult to deploy the client certificate on iOS. I used Apple Configurator and created a profile with the cert to deploy in the past. To do so you'll have to create a new profile and then add the cert to it.
image

To create the certs I use easyrsa which is IMHO the easiest way to create the ca and certs.

@oziee
Copy link

oziee commented Nov 28, 2023

I think Cloudflare through the tunnel is playing some part maybe..

In the network I can curl with the cert and works but then I might be doing something very wrong with the cert that I install on the phone

@cammurray
Copy link

Is this feature planned to get merged? I would absolutely love this without having to use a hacky workaround!

@jc21 any chance this might find it's way in?

@linost-xx
Copy link

@wrouesnel very nice work with this! Thanks alot.

I just have a question, i have "satisfy any" with certificate and IP ranges, and it kinda works. If I'm on WAN it will require me to preset a valid client certificate, and my LAN is on the Allow access list. I'm not required to present a client certificate when I'm on the LAN but the certificate picker comes up every time. Is this by design?

I feel like it should parse the allow list first, and if it finds a match for the current IP it won't "require" a client certificate when satisfy any is active. I mean, it's not a very big deal, but i think it would be cleaner if the certificate picker didn't show when I'm on my LAN.

BR

@wrouesnel
Copy link
Contributor Author

It's a limitation of the "optional" certificate challenge that implements it. Because nginx can ask for the cert it does. So your browser asks you if you want to present a certificate.

Your browser should remember you clicking "no" and then nginx will allow you through anyway if another challenge matches.

@linost-xx
Copy link

It's a limitation of the "optional" certificate challenge that implements it. Because nginx can ask for the cert it does. So your browser asks you if you want to present a certificate.

Your browser should remember you clicking "no" and then nginx will allow you through anyway if another challenge matches.

I see, and yes it will allow me in with even if i click no. But you're right, it should remember it, I've just spent the night in incognito mode while testing this out :) So in in a real use case it shouldn't matter that much.

As a bonus test-case, it seems that applications that don't support client certificates are able to get access (if IP are in allow list) without any interaction about certificates.

Thanks again 👍

@AndrewThrift
Copy link

@jc21 can this feature please be merged.

@svenwanzenried
Copy link

I would really like to see this feature merged. As far as I can see it would be ready to. @jc21 Is there something standing in the way?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
requires-verification Waiting for one or more people to confirm the fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add client certificate support