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

Improve mTLS error messages upon wrong field values in the Ankaios CLI #326

Closed
2 tasks
inf17101 opened this issue Jul 25, 2024 · 3 comments
Closed
2 tasks
Assignees
Labels
enhancement New feature or request. Issue will appear in the change log "Features"
Milestone

Comments

@inf17101
Copy link
Contributor

inf17101 commented Jul 25, 2024

Description

Currently, if the user enters a wrong path for the certificates or key in an Ankaios CLI, then the error message is "Channel preliminary closed." which is not helpful to the user since it is an internal Rust channel error message. This is independent of using the environment variables or the cli arguments directly.

  1. Execute the script tools/certs/create_certs.sh to create certificates for development
  2. Start the server and agent with certificates
  3. Execute an ank cli command and provide an invalid file path to some of the arguments, e.g.:
./ank -v --ca_pem .certs/ca.pem --crt_pem .certs/cli.pem --key_pem asdfg get workloads

You should see the following error message:

image

Goals

Enhance the Ankaios CLI to output helpful error messages when an mTLS setting has an invalid value.

Final result

Summary

To be filled when the final solution is sketched.

Tasks

  • Output "file path does not exist" as error message
  • Analyze if not just the invalid file path can be output, but to output a Certificate Error even it is rejected due to other reasons (Maybe this needs to be included into the request handling and the communication middleware)
@inf17101 inf17101 added the enhancement New feature or request. Issue will appear in the change log "Features" label Jul 25, 2024
@inf17101 inf17101 changed the title Improve mTLS error messages Improve mTLS error messages upon wrong field values Jul 25, 2024
@inf17101
Copy link
Contributor Author

inf17101 commented Jul 25, 2024

The reason is that the polling in the cli does not know when certificates are rejected and the from_server channel object is in this case None since the channel is closed:

    // server_connection.rs:117
    let poll_complete_state_response = async {
            loop {
                match self.from_server.recv().await {
                    Some(FromServer::Response(Response {
                        request_id: received_request_id,
                        response_content: ResponseContent::CompleteState(res),
                    })) if received_request_id == request_id => return Ok(res),
                    None => return Err("Channel preliminary closed."),
                    Some(message) => {
                        // [impl->swdd~cli-stores-unexpected-message~1]
                        self.missed_from_server_messages.push(message);
                    }
                }
            }
        };

Maybe we need proper response handling so that the communication middleware can return a certificate error, then we can also distinguish in the CLI between the errors better and output the user any kind of certificate rejection error (invalid cipher set , what else ...) At a minimum we shall communicate to the user that the file path of the certificate is not valid if a wrong path is provided as argument and if there are some other issues with the certificates then we shall output at least that the communication has failed due to a certificate error (and optional some reason depending on the effort).

@inf17101 inf17101 changed the title Improve mTLS error messages upon wrong field values Improve mTLS error messages upon wrong field values in the Ankaios CLI Jul 25, 2024
@krucod3 krucod3 added this to the v0.5 milestone Aug 6, 2024
@GabyUnalaq
Copy link
Contributor

GabyUnalaq commented Sep 6, 2024

Currently working on this.

The reason that I found is that the error is indeed produced, but ignored at a higher level, inside grpc/src/client.rs - GRPCCommunicationsClient::run().

@GabyUnalaq
Copy link
Contributor

This issue was solved and solution merged. It can be closed now.

@krucod3 krucod3 closed this as completed Sep 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request. Issue will appear in the change log "Features"
Projects
None yet
Development

No branches or pull requests

3 participants