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

mtls over grpc #291

Open
wants to merge 61 commits into
base: main
Choose a base branch
from
Open

mtls over grpc #291

wants to merge 61 commits into from

Conversation

lingnoi
Copy link
Contributor

@lingnoi lingnoi commented Jun 11, 2024

Issues: #6

Definition of Done

The PR shall be merged only if all items mentioned in CONTRIBUTING.md have been followed. In case an item is not applicable as described, please provide a short explanation in the description.

@windsource windsource changed the title 6 mtls over grpc mtls over grpc Jun 11, 2024
agent/src/cli.rs Outdated Show resolved Hide resolved
ank/src/cli.rs Outdated Show resolved Hide resolved
grpc/Cargo.toml Outdated Show resolved Hide resolved
lingnoi and others added 6 commits June 11, 2024 13:42
Co-authored-by: Holger Dormann <holger.dormann@elektrobit.com>
Co-authored-by: Holger Dormann <holger.dormann@elektrobit.com>
agent/src/cli.rs Outdated Show resolved Hide resolved
@windsource

This comment was marked as resolved.

agent/src/cli.rs Outdated Show resolved Hide resolved
tools/create_certs.sh Outdated Show resolved Hide resolved
tools/dev_scripts/ankaios-start Outdated Show resolved Hide resolved
tools/dev_scripts/ankaios-start Show resolved Hide resolved
@maturar
Copy link
Contributor

maturar commented Jul 2, 2024

How is it with requirements? I see neither new requirements nor mapping existing requirements to the new code and tests. This surprises me.

@lingnoi
Copy link
Contributor Author

lingnoi commented Jul 2, 2024

How is it with requirements? I see neither new requirements nor mapping existing requirements to the new code and tests. This surprises me.

There is no surprise, as this PR is not ready for review yet, so the requirements will come.

Copy link
Contributor

@maturar maturar left a comment

Choose a reason for hiding this comment

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

I have reviewed the implementation. SwDD needs to be reviewed yet.

agent/src/runtime_connectors/podman/podman_runtime.rs Outdated Show resolved Hide resolved
grpc/tests/grpc_test.rs Outdated Show resolved Hide resolved
grpc/src/grpc_agent_connection.rs Outdated Show resolved Hide resolved
tools/dev_scripts/ankaios-start Outdated Show resolved Hide resolved

Status: approved

When the root, the server's certificate and the key is provided upon start of the gRPC server, the gRPC server shall use the provided certificates and the key to activate mTLS for the gRPC communication between the involved parties.
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not understand the beginning "When the root,...". Do you mean when the user is running Ankaios (server or agent) as root?
This is valid also for another requirements below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a listing "the root, the server's certificate and the key ...". But still there is a mistake, it should be "the root, the server's certificate and the key are" instead of "the root, the server's certificate and the key is"


Rationale:

To avoid complexity, coming with mTLS configuration e.g. certificates generation and management, during development phase, mTLS can be activated on demand.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to write it with every requirement? What about writing this rationale to the SwDD as design decision (and remove it from here)?


Rationale:

The advantage of using a PEM file is due to its text-based, human-readable format, making it more versatile, as it can contain certificates, private keys, public keys and even certificate chain, compared to DER.
Copy link
Contributor

Choose a reason for hiding this comment

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

What about writing this as design decision?

Before this change ca-key.pem has been stored in /etc/ankaios/certs but
as it is not needed during runtime, the generation has been changed to
store it in the local directory.

Issue-Id: #6
@windsource
Copy link
Contributor

I have tested the tutorial "Setting up Ankaios with mTLS" again and when starting the ank-server it fails with the error message:

Jul 12 12:51:14 vancouver ank-server[41724]: [2024-07-12T10:51:14Z ERROR] server error: 'Certificate error: 'The given certificate file "/etc/ankaios/certs/ca.pem" has incorrect permissions!''

That message does not make sense. The only secret parts of the certificates are the key files, i.e. those files as passed with the --key_pem. We should only check the key files for permissions and not the other certifcate files.

The rest of the files, i.e. those which are passed with --ca_pem and --crt_pem only contain public information and should not to be checked for file permissions.

removed expect usage
@lingnoi
Copy link
Contributor Author

lingnoi commented Jul 15, 2024

I have tested the tutorial "Setting up Ankaios with mTLS" again and when starting the ank-server it fails with the error message:

Jul 12 12:51:14 vancouver ank-server[41724]: [2024-07-12T10:51:14Z ERROR] server error: 'Certificate error: 'The given certificate file "/etc/ankaios/certs/ca.pem" has incorrect permissions!''

That message does not make sense. The only secret parts of the certificates are the key files, i.e. those files as passed with the --key_pem. We should only check the key files for permissions and not the other certifcate files.

The rest of the files, i.e. those which are passed with --ca_pem and --crt_pem only contain public information and should not to be checked for file permissions.

Permission check is now done only for key_pem file!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants