GH-177 - Adds support for DER formatted certficates #336
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| from pathlib import Path | ||
|
|
||
| BINARY_ENCODED_FILETYPES = [".cer", ".der"] | ||
| TEXT_ENCODED_FILETYPES = [".ca-bundle", ".crt", ".key", ".pem"] | ||
|
|
||
|
|
||
| def read_certificate_file(location: str): | ||
|
There was a problem hiding this comment. If we are changing cacert to location then we need to be internally consistent and change existing cacert to something like cacert_location. There was a problem hiding this comment. Going down that route can potentially break backwards compatibility so that's an important aspect to consider. There was a problem hiding this comment. Hey @bcwu - could you help clarify your comment. Are you suggesting that I rename the variables named There was a problem hiding this comment. I did a little more digging. It looks like There was a problem hiding this comment. I was thinking about a couple of things:
There was a problem hiding this comment. I checked the usage of |
||
| """Reads a certificate file from disk. | ||
|
|
||
| The file type (suffix) is used to determine the file encoding. | ||
| Assumption are made based on standard SSL practices. | ||
|
|
||
| Files ending in '.cer' and '.der' are assumed DER (Distinguished | ||
| Encoding Rules) files encoded in binary format. | ||
|
|
||
| Files ending in '.ca-bundle', '.crt', '.key', and '.pem' are PEM | ||
| (Privacy Enhanced Mail) files encoded in plain-text format. | ||
| """ | ||
|
|
||
| path = Path(location) | ||
| suffix = path.suffix | ||
|
There was a problem hiding this comment. 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. There was a problem hiding this comment. 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? There was a problem hiding this comment. We use mimetypes to infer some file types. It may not have full coverage of certificates, but it's a good starting point. There was a problem hiding this comment. 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? There was a problem hiding this comment.
|
||
|
|
||
| if suffix in BINARY_ENCODED_FILETYPES: | ||
| with open(path, "rb") as file: | ||
| return file.read() | ||
|
|
||
| if suffix in TEXT_ENCODED_FILETYPES: | ||
| with open(path, "r") as file: | ||
| return file.read() | ||
|
|
||
| types = BINARY_ENCODED_FILETYPES + TEXT_ENCODED_FILETYPES | ||
| types = sorted(types) | ||
| types = [f"'{_}'" for _ in types] | ||
| human_readable_string = ", ".join(types[:-1]) + ", or " + types[-1] | ||
| raise RuntimeError( | ||
| f"The certificate file type is not recognized. Expected {human_readable_string}. Found '{suffix}'." | ||
| ) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| from tempfile import NamedTemporaryFile | ||
| from unittest import TestCase | ||
|
|
||
| from rsconnect.certificates import read_certificate_file | ||
|
|
||
|
|
||
| class ParseCertificateFileTestCase(TestCase): | ||
|
|
||
| def test_parse_certificate_file_ca_bundle(self): | ||
| res = read_certificate_file("tests/testdata/certificates/localhost.ca-bundle") | ||
| self.assertTrue(res) | ||
|
|
||
| def test_parse_certificate_file_cer(self): | ||
| res = read_certificate_file("tests/testdata/certificates/localhost.cer") | ||
| self.assertTrue(res) | ||
|
|
||
| def test_parse_certificate_file_crt(self): | ||
| res = read_certificate_file("tests/testdata/certificates/localhost.crt") | ||
| self.assertTrue(res) | ||
|
|
||
| def test_parse_certificate_file_der(self): | ||
| res = read_certificate_file("tests/testdata/certificates/localhost.der") | ||
| self.assertTrue(res) | ||
|
|
||
| def test_parse_certificate_file_key(self): | ||
| res = read_certificate_file("tests/testdata/certificates/localhost.key") | ||
| self.assertTrue(res) | ||
|
|
||
| def test_parse_certificate_file_pem(self): | ||
| res = read_certificate_file("tests/testdata/certificates/localhost.pem") | ||
| self.assertTrue(res) | ||
|
|
||
| def test_parse_certificate_file_csr(self): | ||
| with self.assertRaises(RuntimeError): | ||
| read_certificate_file("tests/testdata/certificates/localhost.csr") | ||
|
|
||
| def test_parse_certificate_file_invalid(self): | ||
| with NamedTemporaryFile() as tmpfile: | ||
| with self.assertRaises(RuntimeError): | ||
| read_certificate_file(tmpfile.name) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIDETCCAfkCFGouyhXhN5LZGv4+gVbr+IXMNIB+MA0GCSqGSIb3DQEBCwUAMEUx | ||
| CzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRl | ||
| cm5ldCBXaWRnaXRzIFB0eSBMdGQwHhcNMjMwMTE4MTkwMTA0WhcNMjQwMTE4MTkw | ||
| MTA0WjBFMQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UE | ||
| CgwYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMIIBIjANBgkqhkiG9w0BAQEFAAOC | ||
| AQ8AMIIBCgKCAQEAw8rWVc8S3nU86ybTRpjus3+AvSoQF7yf5yvFzcm8lnNptOsA | ||
| xP9l4NnnWwdJv87JtUAAdL0MxRrWuhsZFAJh5aCMOGL5mdib80dIy7gqNrg6H2lq | ||
| 6ciM5yEpcJo29qtEjfExmRYHNQRbd5OjhggzAj2oCLXPQqSRNVfsmNsm/AxkdVXf | ||
| tJzOEEulb1yh9FMqn8skWxbFuuua/iVzxX5/r9R8BVdsFNlH69cwV2nI/OAOySHg | ||
| zHwWZKVUtQfOkv9jm6CCzpLRrnqarvXt6GmaQFGuqFaAebjPMlp/53csMWQCsrp7 | ||
| Psy/JXKUJ3Dogk2PvziEgzGg2Nf2f2lKXr3WZQIDAQABMA0GCSqGSIb3DQEBCwUA | ||
| A4IBAQB3UsUO5XWlzaO6LsGFCsNbHxH+LxJsejGmnABQt6qzwFF1fs6ixl0RkUsE | ||
| 6/wKKGZdZw9fDotQeDB7zYfQmOqVJtBh/yrmBsW9qzBIJTp/0RSlNsYueyrnGMi5 | ||
| +3g+KHLhnOD9tvnPiz8Haoln2XaGM8iZ+HlVUHxHViWqKTDRcBOmtjglFHmsNy+S | ||
| UFYqgjVbRZNLWOhkC+7LMQutOzfPbO/5zrCSc4nUUhWEYa4AsmYKKMu10VH/EXtB | ||
| QgkX8ZIRa1P8iIe1YDghyjEKBU1WCR7bvbryMTp0HjbSGCuNiiddMesPdwkmPH5/ | ||
| Y/tWij/h+jS8s6uPtyz4KQUcP7gg | ||
| -----END CERTIFICATE----- | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIDETCCAfkCFAro99muVq+KLo92JIXJVyMtLnmBMA0GCSqGSIb3DQEBCwUAMEUx | ||
| CzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRl | ||
| cm5ldCBXaWRnaXRzIFB0eSBMdGQwHhcNMjMwMTE4MTkwMzQwWhcNMjQwMTE4MTkw | ||
| MzQwWjBFMQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UE | ||
| CgwYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMIIBIjANBgkqhkiG9w0BAQEFAAOC | ||
| AQ8AMIIBCgKCAQEAw8rWVc8S3nU86ybTRpjus3+AvSoQF7yf5yvFzcm8lnNptOsA | ||
| xP9l4NnnWwdJv87JtUAAdL0MxRrWuhsZFAJh5aCMOGL5mdib80dIy7gqNrg6H2lq | ||
| 6ciM5yEpcJo29qtEjfExmRYHNQRbd5OjhggzAj2oCLXPQqSRNVfsmNsm/AxkdVXf | ||
| tJzOEEulb1yh9FMqn8skWxbFuuua/iVzxX5/r9R8BVdsFNlH69cwV2nI/OAOySHg | ||
| zHwWZKVUtQfOkv9jm6CCzpLRrnqarvXt6GmaQFGuqFaAebjPMlp/53csMWQCsrp7 | ||
| Psy/JXKUJ3Dogk2PvziEgzGg2Nf2f2lKXr3WZQIDAQABMA0GCSqGSIb3DQEBCwUA | ||
| A4IBAQC+Fx+w7kWmvbwrduzYqKW4k8oFFrjolp1x9Hw7cbpx0qoF5sQadOaYMSt2 | ||
| sBbdZ0qGqlYbRSOYlK9CbXWfzb2Q6ibi6JqkWU+05NYQecj/p1uEwHOxbsvyV2dt | ||
| SEGYkJzyRhxS0gI97A4ati7uQ57ptM2LcsVK2Fu+C3DlpU9FrIXnvZj3I+S+WZ7s | ||
| sYmqv2/Rf8z+Sy0qhxVwbHslKWuQJpXoUpXwOpZNiXgfV1VnjMTaWaagF0MobsHT | ||
| MzE6Dj6f6v2NkfQj7gNQs9ueg4uaACRlUzM/E6Xx1eCL2VXl5nX1ytqW4gKFvWF7 | ||
| xeuMzMhz0EsvN4hhQKEMWDH7p31q | ||
| -----END CERTIFICATE----- |
Binary file not shown.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| -----BEGIN CERTIFICATE----- | ||
| MIIDETCCAfkCFGouyhXhN5LZGv4+gVbr+IXMNIB+MA0GCSqGSIb3DQEBCwUAMEUx | ||
| CzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRl | ||
| cm5ldCBXaWRnaXRzIFB0eSBMdGQwHhcNMjMwMTE4MTkwMTA0WhcNMjQwMTE4MTkw | ||
| MTA0WjBFMQswCQYDVQQGEwJBVTETMBEGA1UECAwKU29tZS1TdGF0ZTEhMB8GA1UE | ||
| CgwYSW50ZXJuZXQgV2lkZ2l0cyBQdHkgTHRkMIIBIjANBgkqhkiG9w0BAQEFAAOC | ||
| AQ8AMIIBCgKCAQEAw8rWVc8S3nU86ybTRpjus3+AvSoQF7yf5yvFzcm8lnNptOsA | ||
| xP9l4NnnWwdJv87JtUAAdL0MxRrWuhsZFAJh5aCMOGL5mdib80dIy7gqNrg6H2lq | ||
| 6ciM5yEpcJo29qtEjfExmRYHNQRbd5OjhggzAj2oCLXPQqSRNVfsmNsm/AxkdVXf | ||
| tJzOEEulb1yh9FMqn8skWxbFuuua/iVzxX5/r9R8BVdsFNlH69cwV2nI/OAOySHg | ||
| zHwWZKVUtQfOkv9jm6CCzpLRrnqarvXt6GmaQFGuqFaAebjPMlp/53csMWQCsrp7 | ||
| Psy/JXKUJ3Dogk2PvziEgzGg2Nf2f2lKXr3WZQIDAQABMA0GCSqGSIb3DQEBCwUA | ||
| A4IBAQB3UsUO5XWlzaO6LsGFCsNbHxH+LxJsejGmnABQt6qzwFF1fs6ixl0RkUsE | ||
| 6/wKKGZdZw9fDotQeDB7zYfQmOqVJtBh/yrmBsW9qzBIJTp/0RSlNsYueyrnGMi5 | ||
| +3g+KHLhnOD9tvnPiz8Haoln2XaGM8iZ+HlVUHxHViWqKTDRcBOmtjglFHmsNy+S | ||
| UFYqgjVbRZNLWOhkC+7LMQutOzfPbO/5zrCSc4nUUhWEYa4AsmYKKMu10VH/EXtB | ||
| QgkX8ZIRa1P8iIe1YDghyjEKBU1WCR7bvbryMTp0HjbSGCuNiiddMesPdwkmPH5/ | ||
| Y/tWij/h+jS8s6uPtyz4KQUcP7gg | ||
| -----END CERTIFICATE----- |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| -----BEGIN CERTIFICATE REQUEST----- | ||
| MIICijCCAXICAQAwRTELMAkGA1UEBhMCQVUxEzARBgNVBAgMClNvbWUtU3RhdGUx | ||
| ITAfBgNVBAoMGEludGVybmV0IFdpZGdpdHMgUHR5IEx0ZDCCASIwDQYJKoZIhvcN | ||
| AQEBBQADggEPADCCAQoCggEBAMPK1lXPEt51POsm00aY7rN/gL0qEBe8n+crxc3J | ||
| vJZzabTrAMT/ZeDZ51sHSb/OybVAAHS9DMUa1robGRQCYeWgjDhi+ZnYm/NHSMu4 | ||
| Kja4Oh9paunIjOchKXCaNvarRI3xMZkWBzUEW3eTo4YIMwI9qAi1z0KkkTVX7Jjb | ||
| JvwMZHVV37SczhBLpW9cofRTKp/LJFsWxbrrmv4lc8V+f6/UfAVXbBTZR+vXMFdp | ||
| yPzgDskh4Mx8FmSlVLUHzpL/Y5uggs6S0a56mq717ehpmkBRrqhWgHm4zzJaf+d3 | ||
| LDFkArK6ez7MvyVylCdw6IJNj784hIMxoNjX9n9pSl691mUCAwEAAaAAMA0GCSqG | ||
| SIb3DQEBCwUAA4IBAQCbHHGn119/PjC7gd18PEGYGss/7rSylpo8FlHT28SI/YjJ | ||
| HTO2ebD0jU4nHcJo8ihOTBUaGRhzURXMX81ernRnFMTy87XKA7FDt5HXLX3i5EYC | ||
| 89S3S8ncaDHC99D1vG4hGAAFuUYi8JWipKaCBKgoHm6VsYqCQeVOPll7PSC3Szyc | ||
| aWR/emEcz/tHXG5a6it6PsWo3jKDPeSPDUtlzZcUz4ssbQih8uEdoTBKwk4mnaCc | ||
| XVndRRfPnhHMOr9BdCjd+LYU0PRtJpsv7pqbHPTQEPEYh7GD99d5jDKtff6J4YkM | ||
| ljLCS+SOPMMTJTEaGuR7NvuIcoj4vODOSEkSUxv9 | ||
| -----END CERTIFICATE REQUEST----- |
Binary file not shown.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cacert should be handled as a file, hence IO; whereas cadata is text, hence str
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
strorbytes, 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