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

Support inline TLS Certificates and Keys #343

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

tisba
Copy link
Collaborator

@tisba tisba commented Feb 4, 2019

This PR adds <cert> and <key> under <set_option name="certificate">:

<set_option name="certificate">
  <certificate>
    <cert>
      <![CDATA[
      -----BEGIN CERTIFICATE-----
      <!-- ... -->
      -----END CERTIFICATE-----
      ]]>
    </cert>
    <key>
      <![CDATA[
      -----BEGIN RSA PRIVATE KEY-----
      <!-- ... -->
      -----END RSA PRIVATE KEY-----
      ]]>
    </key>
  </certificate>
</set_option>

Previously it was only possible to provide certificates and keys via files. I need to be able to handle certificates and keys based on dynvar data (e.g. by extracting from responses or using file servers).

Notes

  • only one certificate and one key is currently supported
  • data has to be provided in PEM encoded format
  • cert and key will be run through ts_search:subst/2 thus supporting to have dynamic configuration
  • New error counter events have been added
    • error_connect_option_* if connection options are invalid (happens in certain cases where the provided certificate or key is not valid)
    • error_connect_tls_bad_certificate if the provided certificate could not be used

@tisba
Copy link
Collaborator Author

tisba commented Feb 4, 2019

My favourite configuration format would be like this:

<set_option name="certificate">
  <cacertificate>
      <![CDATA[
      -----BEGIN CERTIFICATE-----
      <!-- ... -->
      -----END CERTIFICATE-----
      ]]>
  </cacertificate>
  <certificate>
      <![CDATA[
      -----BEGIN CERTIFICATE-----
      <!-- ... -->
      -----END CERTIFICATE-----
      ]]>
  </certificate>
  <private_key password="secret">
    <![CDATA[
      -----BEGIN RSA PRIVATE KEY-----
      <!-- ... -->
      -----END RSA PRIVATE KEY-----
    ]]>
  </private_key>
</set_option>

or, when referencing files:

<set_option name="certificate">
  <cacertificate file="/path/to/ca_cert.pem">
  <certificate file="/path/to/cert.pem">
  <private_key file="/path/to/key.pem" password="secret">
</set_option>

That would be a bigger change to how the existing <certificate> works though.

Any ideas, suggestions or opinions, @nniclausse?

@tisba tisba changed the title Feature: Support inline TLS Certificates and Keys Support inline TLS Certificates and Keys Feb 13, 2019
@tisba
Copy link
Collaborator Author

tisba commented Feb 13, 2019

Any preferences, @nniclausse?

@nniclausse
Copy link
Contributor

The patch looks good; i don't see a good reason to change the format though. Or am i missing something ?

@tisba
Copy link
Collaborator Author

tisba commented Feb 14, 2019

Just aesthetics. <certificate><cert>... and <certificate><key>... feels clunky and redundant...

And I just noticed that you cannot provide the CA certificate chain inline as well (I don't need that, that's probably the reason why I forgot).

@nniclausse
Copy link
Contributor

maybe simply using ssl instead of certifcate could be better:
<ssl><certificate> ...<ca_certificate> <key> ... </ssl>

@tisba
Copy link
Collaborator Author

tisba commented Feb 14, 2019

duh, okay, you're right, that makes more sense. No need to have <certificate> be used with multiple semantics at the same time!

I'll try to refactor this over the weekend...

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

Successfully merging this pull request may close these issues.

2 participants