search

hashicorp / go-plugin

Golang plugin system over RPC.
Mozilla Public License 2.0
5.25k stars 450 forks source link

automtls: fix bidirectional communication when AutoMTLS is enabled #193

Closed fairclothjm closed 2 years ago

fairclothjm commented 2 years ago

Currently enabling AutoMTLS when connecting from a plugin process back to the host results in the following error:

transport: authentication handshake failed: x509: “localhost” certificate is not standards compliant

This is similar but not identical to the error described in https://github.com/hashicorp/go-plugin/issues/109 and https://github.com/hashicorp/go-plugin/pull/179

This PR enables the client and server to successfully establish the mTLS connection for bidirectional communication.

calvn commented 2 years ago

Took a look at how we generate certs on Vault for mTLS via bootstrap + wrapping token, and it looks the same as this modulo the issuer addition. Do you get the error if issuer is not specified?

fairclothjm commented 2 years ago

Took a look at how we generate certs on Vault for mTLS via bootstrap + wrapping token, and it looks the same as this modulo the issuer addition. Do you get the error if issuer is not specified?

@calvn Yes, I do get the same error if the Issuer is not specified. But I did some more testing just now and if I don't set the Subject.Organization field then it works without setting the Issuer. generateCert in sdk/helper/pluginutil/tls.go also does not set the Subject.Organization.

Do you think that approach is preferable? I don't see any downside either way

jbardin commented 2 years ago

I haven't thought about this in a long time, so I'm not going to try and comment about what is the more "correct" method for cert generation, but I would be concerned with making sure the changes is compatible between client and server versions. I would verify that existing clients and servers on different minor versions can interoperate with this change in either direction, otherwise we will need some way to make it optional to prevent needing a new major version.

calvn commented 2 years ago

Yes, I do get the same error if the Issuer is not specified. But I did some more testing just now and if I don't set the Subject.Organization field then it works without setting the Issuer. generateCert in sdk/helper/pluginutil/tls.go also does not set the Subject.Organization.

Interesting, I wonder how mTLS is working on Vault today since it doesn't seem to specify Issuer on cert generation.

WRT the organization field in Subject, I'd say that it's fine to leave it since that's been always present on go-plugin and doesn't seem to interfere with mTLS.

fairclothjm commented 2 years ago

I wonder how mTLS is working on Vault today

Database plugins don't use the brokered connections back to the host. Using GRPCBroker is when I am seeing the Certificate errors occur.