Add certificate based authentication to c8y-auth-proxy #2397
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: James Rhodes <[email protected]>
jarhodes314
had a problem deploying
to
Test Pull Request
GitHub Actions
Failure
Codecov Report
Additional details and impacted files
|
Robot Results
Failed Tests
|
There was a problem hiding this comment.
Before going further, it would be good to sketch a user documentation telling how to deploy certificates over misc thin-edge devices for the c8y_auth_proxy to serve only HTTPS requests to authenticated services. Such a documentation would really help to clarify the design and understand the code.
| @@ -111,6 +111,7 @@ rand = "0.8" | |||
| rcgen = { version = "0.9", features = ["pem", "zeroize"] } | |||
| regex = "1.4" | |||
| reqwest = { version = "0.11", default-features = false } | |||
| ring = "0.16" | |||
There was a problem hiding this comment.
Note that it would be better to avoid a direct dependency on ring as this crate supports fewer architectures than we would ideally like to support.
| .context("creating identity from configured client certificate and private key") | ||
| }) | ||
| .transpose()? | ||
| .unwrap(); |
There was a problem hiding this comment.
This unwrap() panics when running the agent with no specific certificate settings.
tedge-agent should be able to run with no Identity. Sure, it will be then unable to download software from the c8y auth proxy if the latter has been configured to enforce client authentication. But, it should work with a mapper that accepts anonymous connections.
There was a problem hiding this comment.
Nice!
However, this improvement of the fan_in_message_type macro deserves its own PR ... to be merged sooner :-)
It would also be good to have an example in the doc comment.
| #[derive(Error, Debug)] | ||
| #[derive(Error, Debug, miette::Diagnostic)] |
There was a problem hiding this comment.
I would prefer to have a specific PR introducing miette.
So we can discuss the pros and cons of using it and conclude,
rather than have a mix of miette and anyhow proponents and a bloated software.
Comment on lines
-86
to
+107
| pub enum Auth { | ||
| pub enum RequiredAuth { | ||
| /// HTTP Bearer authentication | ||
| Bearer(String), | ||
| /// TLS certificate authentication | ||
| Certificate, | ||
| } |
There was a problem hiding this comment.
I don't understand this change. This is even going in the reverse direction I was expecting.
- In their first version,
tedge-mapperandtedge-agentwere exchanging over MQTT the credentials required to download a software package from c8y. This was not only a security issue (theBearerbeing sent clear-text over MQTT), but also fragile (as the JWT token might expired before being used). - The introduction of
c8y_auth_proxymakes the authentication to c8y an internal issue of the mapper. This is less fragile, but still not secure as any local process has full access to the c8y API which is exposed over non-TLS HTTP. - This PR aims to add HTTPS support with mutual TLS authentication of the mapper and the agent (the agent is possibly running on a different device). Each needs its own certificate (and private key). Each needs to trust the certificate of the other. But no identity and even no credentials have to be exchanged over MQTT between the mapper and the agent.
- Hence, I was expecting the
enum Authto be deprecated, not augmented.
11 tasks
|
This is now entirely superseeded by #2430 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
This adds HTTPS support and (opt-in) certificate-based authentication to the c8y auth proxy.
At the moment this is a draft PR as the agent currently doesn't handle the device certificate not being present. It also doesn't yet document the API changes (HTTP -> HTTPS), nor does it touch the integration tests.
Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments
When the only authentication scheme available for software updates was bearer tokens, the mapper simply supplied the token along with the request, which the agent would simply use. As this adds certificate authentication, the mapper requires the agent to also use certificate authentication (if configured). This has the advantage of us being able to delegate authentication largely to a trusted library, but does add complexity in the agent code and adds possible error states, due to the way certificates need to be configured.
The most significant changes to the agent centre around the generic authentication strategy supported by
DownloadInfo.DownloadInfohas been modified to support an incoming request from the mapper (which would simply tell the agent to use a certificate, if required), a request that is being processed (at which point the agent will add the required certificate) and a response (which has the authentication anonymised for security/because we cannot clonereqwest::Identity, which represents the certificate/key pair in use).Due to this complexity, it may be desirable to add an alternative authentication scheme to the auth proxy. This could involve the mapper generating its own bearer token, and the agent consuming this as it does currently. This would ensure backwards compatibility with the existing software update API (if someone is using a custom agent implementation). It would also remove the burden of the agent (possibly) needing to supply certificates, which is difficult to handle errors for proactively before a user attempts to e.g. update software and finds the agent configuration is broken. The difficulty arises from the fact certificates may not be required, either due to not specifying trusted certificates for the auth proxy, or not connecting thin-edge to Cumulocity (in which case the auth proxy isn't running). If the mapper could decide itself how to authenticate a request to itself (e.g. by creating a JWT using the certificate used for the HTTPS connection), this alleviates the need for the agent to care how the request should be authenticated. Certificate-based authentication would still be useful for thin-edge users to connect to the API themselves (if no certificates are provided, the mapper could just disable authentication entirely, both for external users and agents).