GH-177 - Adds support for DER formatted certficates #336

tdstein · 2023-01-17T20:40:20Z

Description

Modifies the --cacert option to support binary-encoded certificates,
such as DER (Distinguished Encoding Rules) format. This change is
backwards compatible and has no effect on text-encoded certificate.

Additionally, file-type checks have been added for common certificate
file types. This includes '.cer', '.der', '.ca-bundle', '.crt',
'.key', and '.pem'.

Connected to #177

Testing Notes / Validation Steps

The included unit tests validate the suffix routing for each type of certificate.

Additional manual testing is required to validate that requests using the --cacert option are successful. To facilate testing of DER certificiate types, I have generate a valid DER file in this pull request: https://github.com/rstudio/connect/pull/22753

To validate DER formatting:

rsconnect details --api-key $CONNECT_API_KEY --server https://alwayssecure.rsc:3443 --cacert alwayssecure.rsc.der

The output should be identical to the same call with the existing CRT file.

rsconnect details --api-key $CONNECT_API_KEY --server https://alwayssecure.rsc:3443 --cacert alwayssecure.rsc.crt

bcwu · 2023-01-18T20:11:38Z

rsconnect/api.py

@@ -360,7 +360,7 @@ def __init__(
        url: str = None,
        api_key: str = None,
        insecure: bool = False,
-        cacert: IO = None,


cacert should be handled as a file, hence IO; whereas cadata is text, hence str

Correct. Here the cacert variable is a string representing the file path, instead of a file pointer. The call on line 436 opens the file pointer, which was originally handled by Click), and reads the file contents in either text mode or byte mode depending on the file-type.

The existing implementation assumes that the file pointer has been opened in text mode, which doesn't work when using DER files since they are encoded in binary format. L436 onwards, cadata is either a str or bytes, both of which are accepted when creating an SSLContext. This is the same as the existing implementation.

The Click documentation goes into scenario a little further: https://click.palletsprojects.com/en/8.1.x/arguments/#file-path-arguments

bcwu · 2023-01-18T20:17:09Z

rsconnect/certificates.py

+TEXT_ENCODED_FILETYPES = [".ca-bundle", ".crt", ".key", ".pem"]
+
+
+def read_certificate_file(location: str):


If we are changing cacert to location then we need to be internally consistent and change existing cacert to something like cacert_location.

Going down that route can potentially break backwards compatibility so that's an important aspect to consider.

Hey @bcwu - could you help clarify your comment. Are you suggesting that I rename the variables named cacert to cacert_location so that the variable type is more obvious?

I did a little more digging. It looks like cafile is the typical nomenclature for file name references. https://docs.python.org/3/library/ssl.html

I was thinking about a couple of things:

Whether changing the type hints of cacert (from IO to str) will break any existing behavior

In this case either way (type hint set to IO or str) is probably all right. In general it helps to think through whether changes are backwards compatible.

Possibly rename cacert if we do change the type hint

this will probably cause more confusion than it clarifies. It will certainly break existing behavior; so, on second thought not a good idea, please disregard.

I checked the usage of cacert in each of the methods where the type signature is changed. In each case the variable is only used in combination with read_certificate_file. So, this shouldn't break any exisiting behavior.

Modifies the --cacert option to support binary-encoded certificates, such as DER (Distinguished Encoding Rules) format. This change is backwards compatible and has no effect on text-encoded certificate. Additionally, file-type checks have been added for common certificate file types. This includes '.cer', '.der', '.ca-bundle', '.crt', '.key', and '.pem'.

bcwu · 2023-01-19T19:09:14Z

rsconnect/certificates.py

+    """
+
+    path = Path(location)
+    suffix = path.suffix


Checking the file type by examining its magic number is a safer way of inference than checking the suffix. This especially pertains to security related files like certificates.

Good idea. I'll look into this a bit.

Do you happen to know of any documentation that shows how to accomplish this across each of the certificate types?

We use mimetypes to infer some file types. It may not have full coverage of certificates, but it's a good starting point.

It doesn't appear that certificates contain standard header blocks, but PEM files and DER files both adhere to a standard. https://www.openssl.org/docs/man3.0/man1/openssl-format-options.html

I can add additional validation that inspects the contents to check expecatations based on this documentation. But, I'm not sure if that is necessary.

@mmarchetti - do you have any additional insights into this?

mimetypes determines types by file suffix, so it's probably equivalent to what we're doing. I don't think we need to inspect file contents - if an invalid certificate file is provided, it will fail with an SSL error when we try to connect.

kgartland-rstudio · 2023-01-24T21:56:40Z

Verified fix.

Reproduced the issue on 1.14.0:

> rsconnect deploy fastapi ./ -s https://alwayssecure:3443 -k {API_KEY} --cacert ../../ssl/alwayssecure.rsc.der
    Warning: the existing manifest.json file will not be used or considered.
Traceback (most recent call last):
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/site-packages/rsconnect/main.py", line 91, in wrapper
    result = func(*args, **kwargs)
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/site-packages/rsconnect/main.py", line 1196, in deploy_app
    ce = RSConnectExecutor(**kwargs)
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/site-packages/rsconnect/api.py", line 376, in __init__
    self.setup_remote_server(
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/site-packages/rsconnect/api.py", line 436, in setup_remote_server
    ca_data = text_type(cacert.read())
  File "/Users/kgartland/.pyenv/versions/3.8.2/lib/python3.8/codecs.py", line 322, in decode
    (result, consumed) = self._buffer_decode(data, self.errors, final)
UnicodeDecodeError: 'utf-8' codec can't decode byte 0x82 in position 1: invalid start byte
Internal error: 'utf-8' codec can't decode byte 0x82 in position 1: invalid start byte

On this branch (1.14.1.dev3+gf4f56c1), using the same command, the der cert works as expected.

other certs continue to work
works on Windows VM
--insecure option still works as expected without supplying --cacert

tdstein · 2023-01-24T21:58:10Z

@bcwu / @mmarchetti - Now that @kgartland-rstudio has validated the change, am I safe to merge?

bcwu · 2023-01-24T22:16:47Z

@bcwu / @mmarchetti - Now that @kgartland-rstudio has validated the change, am I safe to merge?

Yes, go ahead.

tdstein force-pushed the 177-cacert-option branch 2 times, most recently from 11ef4eb to 1031fe7 Compare January 18, 2023 19:16

tdstein changed the title ~~GH-177 - Adds support for DER-encoded certficates~~ GH-177 - Adds support for DER formatted certficates Jan 18, 2023

tdstein force-pushed the 177-cacert-option branch 5 times, most recently from e0f8c2b to d0de2ca Compare January 18, 2023 19:59

tdstein requested review from mmarchetti and bcwu January 18, 2023 20:06

tdstein marked this pull request as ready for review January 18, 2023 20:06

bcwu reviewed Jan 18, 2023

View reviewed changes

tdstein force-pushed the 177-cacert-option branch from d0de2ca to 81f8fb4 Compare January 19, 2023 15:29

Merge branch 'master' into 177-cacert-option

ec4b5bf

bcwu reviewed Jan 19, 2023

View reviewed changes

mmarchetti approved these changes Jan 23, 2023

View reviewed changes

Merge branch 'master' into 177-cacert-option

f4f56c1

bcwu approved these changes Jan 23, 2023

View reviewed changes

tdstein merged commit 802a761 into master Jan 25, 2023

tdstein deleted the 177-cacert-option branch January 25, 2023 15:18

kgartland-rstudio mentioned this pull request Apr 21, 2023

rsconnect add server with --cacert doesn't work with .der certs #413

Open

		TEXT_ENCODED_FILETYPES = [".ca-bundle", ".crt", ".key", ".pem"]


		def read_certificate_file(location: str):

GH-177 - Adds support for DER formatted certficates #336

GH-177 - Adds support for DER formatted certficates #336

Uh oh!

Conversation

tdstein commented Jan 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing Notes / Validation Steps

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kgartland-rstudio commented Jan 24, 2023

Uh oh!

tdstein commented Jan 24, 2023

Uh oh!

bcwu commented Jan 24, 2023

Uh oh!

Uh oh!

tdstein commented Jan 17, 2023 •

edited

Loading