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

Add support for chained certificates #3134

Merged
merged 3 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,16 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Suma.HttpExecutor do
)
end

def get_cert_der(ca_cert) do
ca_cert
|> :public_key.pem_decode()
|> Enum.map(&:public_key.pem_entry_decode/1)
|> Enum.map(&split_type_and_entry/1)
|> Enum.map(fn {asn1_type, asn1_entry} ->
:public_key.der_encode(asn1_type, asn1_entry)
end)
end

defp request_options(auth, ca_cert),
do: [hackney: [cookie: [auth]]] ++ ssl_options(ca_cert) ++ timeout_options()

Expand All @@ -208,21 +218,10 @@ defmodule Trento.Infrastructure.SoftwareUpdates.Suma.HttpExecutor do
defp ssl_options(nil), do: []

defp ssl_options(ca_cert),
do: [ssl: [verify: :verify_peer, cacerts: [get_cert_der(ca_cert)]]]

def get_cert_der(ca_cert) do
{cert_type, cert_entry} =
ca_cert
|> :public_key.pem_decode()
|> hd()
|> :public_key.pem_entry_decode()
|> split_type_and_entry()

:public_key.der_encode(cert_type, cert_entry)
end
do: [ssl: [verify: :verify_peer, cacerts: get_cert_der(ca_cert)]]

def split_type_and_entry(ans1_entry) do
ans1_type = elem(ans1_entry, 0)
{ans1_type, ans1_entry}
defp split_type_and_entry(asn1_entry) do
asn1_type = elem(asn1_entry, 0)
{asn1_type, asn1_entry}
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: this function could be just declarative:

Suggested change
defp split_type_and_entry(asn1_entry) do
asn1_type = elem(asn1_entry, 0)
{asn1_type, asn1_entry}
defp split_type_and_entry({asn1_type, _} = asn1_entry) do
{asn1_type, asn1_entry}

Copy link
Member Author

@nelsonkopliku nelsonkopliku Nov 8, 2024

Choose a reason for hiding this comment

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

At the end I removed the function altogether and moved it in the mapping pipe 9569dde

Just for reference, the pem decoded entry seems to be a 4 element tuple rather than a 2 element tuple https://www.erlang.org/doc/apps/public_key/public_key.html#t:pem_entry/0.

Hope it remains like this 😄

end
end
16 changes: 16 additions & 0 deletions test/fixtures/cert/one_entry_chain.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
-----BEGIN CERTIFICATE-----
MIICYjCCAcugAwIBAgIJAIwkDNqe2UDmMA0GCSqGSIb3DQEBCwUAMD8xHzAdBgNV
BAoMFkVsaXhpci5YNTA5LlRlc3QuU3VpdGUxHDAaBgNVBAMME0FsdGVybmF0aXZl
IFJvb3QgQ0EwHhcNMjQxMTA3MTU0MDAxWhcNNDkxMTA3MTU0NTAxWjA/MR8wHQYD
VQQKDBZFbGl4aXIuWDUwOS5UZXN0LlN1aXRlMRwwGgYDVQQDDBNBbHRlcm5hdGl2
ZSBSb290IENBMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCzUGvVZbiIJQLE
7PFhyNfsLYtxPouyijxx3MoAWjmDbdSdrBVNk2OaFC3hZLrR6Q9a+vUSVNaQqDVK
3FdmsAsHr1Qv7aPH/5beVnLiIOcZ44Er6bDQnE2m9zd9JWjF0K6/11AIQj7bNlic
IUejsCohFhMFyoAS3Fxs5tkhsnQW8wIDAQABo2YwZDAOBgNVHQ8BAf8EBAMCAYYw
HQYDVR0OBBYEFPTCoPvRToC6zHTW4NddG+a/Ow24MB8GA1UdIwQYMBaAFPTCoPvR
ToC6zHTW4NddG+a/Ow24MBIGA1UdEwEB/wQIMAYBAf8CAQIwDQYJKoZIhvcNAQEL
BQADgYEAOtFdLegvhFqoeVNHvPjOkB6Wlf9XYjAZDB4SssFiIDkGCx2OI/TlICur
Mw3C0yysBWSSSOB7xonpJadf5wbumARAir4xG0YtTi9prFkatdtMtQ0nIEyVXh0G
WD00Yp7OS6SdEbcQ6bCzuc0wmburb+KOPQJ4/h67sCG9lXprc7s=
-----END CERTIFICATE-----

49 changes: 49 additions & 0 deletions test/fixtures/cert/three_entries_chain.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
-----BEGIN CERTIFICATE-----
MIICczCCAdygAwIBAgIJALR2PyifzHdYMA0GCSqGSIb3DQEBCwUAMDMxHzAdBgNV
BAoMFkVsaXhpci5YNTA5LlRlc3QuU3VpdGUxEDAOBgNVBAMMB1Jvb3QgQ0EwHhcN
MjQxMTA3MTU0MDAxWhcNMzQxMTA3MTU0NTAxWjA7MR8wHQYDVQQKDBZFbGl4aXIu
WDUwOS5UZXN0LlN1aXRlMRgwFgYDVQQDDA9JbnRlcm1lZGlhdGUgQ0EwgZ8wDQYJ
KoZIhvcNAQEBBQADgY0AMIGJAoGBALf/GcAKFCZvd3a7BAvPsT2hPYfCFfj33tWt
6J/tw1mMpZL6v0JySEvv7TgKlgCskV6TnYTKwFfrTaeeimrjdNtoRSjsbwJzeGTS
1zmwq+I7d6yZRh3bqaVELdufmB56kj15f2OXAuoTI3gBw63IWu4d5+7aUIYXsH5/
h5TZ55ThAgMBAAGjgYYwgYMwEgYDVR0TAQH/BAgwBgEB/wIBADAOBgNVHQ8BAf8E
BAMCAYYwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMCMB0GA1UdDgQWBBRd
7IOGY9dvei7xBJTUDjKhcvJ5zzAfBgNVHSMEGDAWgBQtUjdaTs6vVmmbR9NpeTdg
WzcAFDANBgkqhkiG9w0BAQsFAAOBgQC/XwlIJUVbx0P7ZxEkXRIHfOp9TBvFmDLz
vqddCAv8Sat4bSaUAYEdqzLpa9GUGJOR4teXasoyKMt9NKBgbX8Q4GiVlJp2fHXt
FmB0lThaSNXM0RdPdQCyLnFY4h3nxTP3rnyviCFRvz+wGPBvR8m+8qeRq4txTEBm
J2Ag8E66CA==
-----END CERTIFICATE-----

-----BEGIN CERTIFICATE-----
MIICdjCCAd+gAwIBAgIIGs3fLTuZTtIwDQYJKoZIhvcNAQELBQAwPzEfMB0GA1UE
CgwWRWxpeGlyLlg1MDkuVGVzdC5TdWl0ZTEcMBoGA1UEAwwTQWx0ZXJuYXRpdmUg
Um9vdCBDQTAeFw0yNDExMDcxNTQwMDFaFw0zNDExMDcxNTQ1MDFaMDMxHzAdBgNV
BAoMFkVsaXhpci5YNTA5LlRlc3QuU3VpdGUxEDAOBgNVBAMMB1Jvb3QgQ0EwgZ8w
DQYJKoZIhvcNAQEBBQADgY0AMIGJAoGBANRlfoVELK6E9eZ7GI0D+45BfQjEixG1
cmPX1xqeFmR4Mr+yeNSdmpW0tcZO8mx/Vd0RaQiVszkmwr+FUSkzIouunsxPU8Hn
nTc6dUM+7EjY86MppZBE5NEL/9pGunsBIlSx7PZjvK5FuYcsDtVK87hOJjCASRUa
9YaodaIUmZzbAgMBAAGjgYYwgYMwDgYDVR0PAQH/BAQDAgGGMB0GA1UdJQQWMBQG
CCsGAQUFBwMBBggrBgEFBQcDAjAdBgNVHQ4EFgQULVI3Wk7Or1Zpm0fTaXk3YFs3
ABQwHwYDVR0jBBgwFoAU9MKg+9FOgLrMdNbg110b5r87DbgwEgYDVR0TAQH/BAgw
BgEB/wIBATANBgkqhkiG9w0BAQsFAAOBgQBF4is4DqvGpVQhwfa53tRkk0WHPoCX
Laslw7qFRKxop9/0tGOCz6gBvra2os5VqJhYCf+0rsFLmOX05Lm107PZwIfjkLwI
zi3WVU+mVIBFPBI/EwwHZ/yOtF/w5x8+sxXpGz5Zj1idBHZpbwiKK+2a2Jebmy8b
5N0Dl4AYIXqYhA==
-----END CERTIFICATE-----

-----BEGIN CERTIFICATE-----
MIICYjCCAcugAwIBAgIJAIwkDNqe2UDmMA0GCSqGSIb3DQEBCwUAMD8xHzAdBgNV
BAoMFkVsaXhpci5YNTA5LlRlc3QuU3VpdGUxHDAaBgNVBAMME0FsdGVybmF0aXZl
IFJvb3QgQ0EwHhcNMjQxMTA3MTU0MDAxWhcNNDkxMTA3MTU0NTAxWjA/MR8wHQYD
VQQKDBZFbGl4aXIuWDUwOS5UZXN0LlN1aXRlMRwwGgYDVQQDDBNBbHRlcm5hdGl2
ZSBSb290IENBMIGfMA0GCSqGSIb3DQEBAQUAA4GNADCBiQKBgQCzUGvVZbiIJQLE
7PFhyNfsLYtxPouyijxx3MoAWjmDbdSdrBVNk2OaFC3hZLrR6Q9a+vUSVNaQqDVK
3FdmsAsHr1Qv7aPH/5beVnLiIOcZ44Er6bDQnE2m9zd9JWjF0K6/11AIQj7bNlic
IUejsCohFhMFyoAS3Fxs5tkhsnQW8wIDAQABo2YwZDAOBgNVHQ8BAf8EBAMCAYYw
HQYDVR0OBBYEFPTCoPvRToC6zHTW4NddG+a/Ow24MB8GA1UdIwQYMBaAFPTCoPvR
ToC6zHTW4NddG+a/Ow24MBIGA1UdEwEB/wQIMAYBAf8CAQIwDQYJKoZIhvcNAQEL
BQADgYEAOtFdLegvhFqoeVNHvPjOkB6Wlf9XYjAZDB4SssFiIDkGCx2OI/TlICur
Mw3C0yysBWSSSOB7xonpJadf5wbumARAir4xG0YtTi9prFkatdtMtQ0nIEyVXh0G
WD00Yp7OS6SdEbcQ6bCzuc0wmburb+KOPQJ4/h67sCG9lXprc7s=
-----END CERTIFICATE-----
33 changes: 33 additions & 0 deletions test/fixtures/cert/two_entries_chain.pem
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
-----BEGIN CERTIFICATE-----
MIICSTCCAbKgAwIBAgIIaNZzHCfEcPQwDQYJKoZIhvcNAQELBQAwMzEfMB0GA1UE
CgwWRWxpeGlyLlg1MDkuVGVzdC5TdWl0ZTEQMA4GA1UEAwwHUm9vdCBDQTAeFw0y
NDExMDcxNTQwMDFaFw00OTExMDcxNTQ1MDFaMDMxHzAdBgNVBAoMFkVsaXhpci5Y
NTA5LlRlc3QuU3VpdGUxEDAOBgNVBAMMB1Jvb3QgQ0EwgZ8wDQYJKoZIhvcNAQEB
BQADgY0AMIGJAoGBANRlfoVELK6E9eZ7GI0D+45BfQjEixG1cmPX1xqeFmR4Mr+y
eNSdmpW0tcZO8mx/Vd0RaQiVszkmwr+FUSkzIouunsxPU8HnnTc6dUM+7EjY86Mp
pZBE5NEL/9pGunsBIlSx7PZjvK5FuYcsDtVK87hOJjCASRUa9YaodaIUmZzbAgMB
AAGjZjBkMBIGA1UdEwEB/wQIMAYBAf8CAQEwDgYDVR0PAQH/BAQDAgGGMB0GA1Ud
DgQWBBQtUjdaTs6vVmmbR9NpeTdgWzcAFDAfBgNVHSMEGDAWgBQtUjdaTs6vVmmb
R9NpeTdgWzcAFDANBgkqhkiG9w0BAQsFAAOBgQCudruo0hLXFayy3vJKJkzSMmqr
2QpfbK2belUu34l9xUfU/m3ueQdcTJysueDXQHz4bsEw2cOEAoCeeXHv04j+k9rX
Qqy3DMN01Z1L2XhBRK4G5vYXdDhOSatAfTWzH9mQeks12SNxjivVi/gZ9ucuFLfT
HtqiiVqmOiFSs7Du6A==
-----END CERTIFICATE-----

-----BEGIN CERTIFICATE-----
MIICczCCAdygAwIBAgIJALR2PyifzHdYMA0GCSqGSIb3DQEBCwUAMDMxHzAdBgNV
BAoMFkVsaXhpci5YNTA5LlRlc3QuU3VpdGUxEDAOBgNVBAMMB1Jvb3QgQ0EwHhcN
MjQxMTA3MTU0MDAxWhcNMzQxMTA3MTU0NTAxWjA7MR8wHQYDVQQKDBZFbGl4aXIu
WDUwOS5UZXN0LlN1aXRlMRgwFgYDVQQDDA9JbnRlcm1lZGlhdGUgQ0EwgZ8wDQYJ
KoZIhvcNAQEBBQADgY0AMIGJAoGBALf/GcAKFCZvd3a7BAvPsT2hPYfCFfj33tWt
6J/tw1mMpZL6v0JySEvv7TgKlgCskV6TnYTKwFfrTaeeimrjdNtoRSjsbwJzeGTS
1zmwq+I7d6yZRh3bqaVELdufmB56kj15f2OXAuoTI3gBw63IWu4d5+7aUIYXsH5/
h5TZ55ThAgMBAAGjgYYwgYMwEgYDVR0TAQH/BAgwBgEB/wIBADAOBgNVHQ8BAf8E
BAMCAYYwHQYDVR0lBBYwFAYIKwYBBQUHAwEGCCsGAQUFBwMCMB0GA1UdDgQWBBRd
7IOGY9dvei7xBJTUDjKhcvJ5zzAfBgNVHSMEGDAWgBQtUjdaTs6vVmmbR9NpeTdg
WzcAFDANBgkqhkiG9w0BAQsFAAOBgQC/XwlIJUVbx0P7ZxEkXRIHfOp9TBvFmDLz
vqddCAv8Sat4bSaUAYEdqzLpa9GUGJOR4teXasoyKMt9NKBgbX8Q4GiVlJp2fHXt
FmB0lThaSNXM0RdPdQCyLnFY4h3nxTP3rnyviCFRvz+wGPBvR8m+8qeRq4txTEBm
J2Ag8E66CA==
-----END CERTIFICATE-----

Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
defmodule Trento.Infrastructure.SoftwareUpdates.Adapter.SumaHttpExecutorTest do
@moduledoc false
nelsonkopliku marked this conversation as resolved.
Show resolved Hide resolved

use ExUnit.Case

alias Trento.Infrastructure.SoftwareUpdates.Suma.HttpExecutor

def load_certificate_content(name) do
File.cwd!()
|> Path.join("/test/fixtures/cert")
|> Path.join(name)
|> File.read!()
end

describe "Http executor certificate handling" do
test "should support chained certificates" do
scenarios = [
%{cert: "one_entry_chain.pem", expected_entries: 1},
%{cert: "two_entries_chain.pem", expected_entries: 2},
%{cert: "three_entries_chain.pem", expected_entries: 3}
]

for %{cert: cert, expected_entries: expected_entries} <- scenarios do
assert cert
|> load_certificate_content()
|> HttpExecutor.get_cert_der()
|> length() == expected_entries
end
end
end
Comment on lines +13 to +28
Copy link
Member

Choose a reason for hiding this comment

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

I'm only left longing for more comprehensive testing, e.g. checking that the CA chain can actually verify a server certificate previously signed with it, but I realize this is a lot of work, and at the moment time is of the essence.

Maybe @gagandeepb could pick this up at a later stage, since he mentioned he wanted to do some testing in the context of our TLS usage in general.

Copy link
Member Author

Choose a reason for hiding this comment

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

Definitely something to look into. Thanks for the pointer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with you @stefanotorresi but at the same time this is also something on which we need a lot of feedback by the users 👍

end
Loading