Skip to content

Conversation

@luoxiaohei
Copy link
Contributor

Due to the limitation of python-grpcio, which only supports the P256 elliptic curve(grpc/grpc#23235), when AutoMTLS is enabled, the Python protocol plugin service cannot communicate properly with the Go Client.

Therefore, I have added the 'AutoMTLSCurve' option to allow specifying the curve type independently for AutoMTLS in the Client and Server. This will facilitate future extensions.

Add a new configuration option `AutoMTLSCurve` to allow users to specify the elliptic curve used in generating the certificate and private key for AutoMTLS.
This improves the extensibility of the project.

The default curve is `elliptic.P521`.
@luoxiaohei
Copy link
Contributor Author

The limitation of python-grpcio only supporting the P256 curve, since fixed by grpc/grpc#34867, means that the Python protocol plugin service can communicate properly with the Go Client using AutoMTLS as of grpc's next release version, without specifying the curve type. Therefore, this pull request may not be as urgent or necessary as before, while it still provides some flexibility for future extensions.

Please let me know if you have any questions or feedback.

Copy link

@sonamtenzin2 sonamtenzin2 left a comment

Choose a reason for hiding this comment

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

Please add test cases

}

if config.AutoMTLSCurve == nil {
config.AutoMTLSCurve = defaultMTLSCurve

Choose a reason for hiding this comment

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

Instead of changing config in client and server, you can add a default scenario in mtls itself

)

// defaultMTLSCurve is the default curve used for generating mTLS certificates.
var defaultMTLSCurve = elliptic.P521()

Choose a reason for hiding this comment

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

This will be removed in case we handle it in the function as stated above

@sonamtenzin2
Copy link

Hi @luoxiaohei, thanks for the update on python change. I have added comments on the PR, you can look into the comments in case you still want to continue with the feature

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.

2 participants