From 18974328d70b6cbd9b8b7201504d15b444720523 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 28 Jul 2026 15:56:50 +0200 Subject: [PATCH 01/10] Add test for issue #69893 --- .../pytests/functional/states/test_x509_v2.py | 97 +++++++++ tests/pytests/functional/utils/test_x509.py | 189 ++++++++++++++++++ 2 files changed, 286 insertions(+) diff --git a/tests/pytests/functional/states/test_x509_v2.py b/tests/pytests/functional/states/test_x509_v2.py index 2c96146018b4..a1c2ef55af29 100644 --- a/tests/pytests/functional/states/test_x509_v2.py +++ b/tests/pytests/functional/states/test_x509_v2.py @@ -4,6 +4,13 @@ import pytest +from tests.pytests.functional.utils.test_x509 import ( # pylint: disable=unused-import + ca_A, + ca_AI, + ca_B, + ca_BI, +) + try: import cryptography import cryptography.x509 as cx509 @@ -180,6 +187,67 @@ def ca_key_enc(): -----END ENCRYPTED PRIVATE KEY-----""" +@pytest.fixture +def ca_sub(): + return """\ +-----BEGIN CERTIFICATE----- +MIIDejCCAmKgAwIBAgIUYJfOr7sQ4QiGVO8/OOe+Jc9RL7cwDQYJKoZIhvcNAQEL +BQAwKzELMAkGA1UEBhMCVVMxDTALBgNVBAMMBFRlc3QxDTALBgNVBAoMBFNhbHQw +HhcNMjYwNzI4MDgwNjU1WhcNMzIxMTEyMTQwNDMzWjAuMQswCQYDVQQGEwJVUzEN +MAsGA1UECgwEU2FsdDEQMA4GA1UEAwwHVGVzdFN1YjCCASIwDQYJKoZIhvcNAQEB +BQADggEPADCCAQoCggEBAM62B1iql/J2d9V642X/UQWmVHzkntnW4ydZa98YHz5a +VQ6/Vawo2vHPxJhOvtBpok+rNG3Yj4VNbE7wD0yWI/FZl9SXe3QnqGRnVwBR1EWc +l/iVKvnknxtub/M8FxE+wjpje2F7p0crujz95y//jzEZqeTVRbTEMQCalUaCkQYR +7B4FL4CdsbeZlAxQ+T0DRMU1JG53aYjV5PrEaWP6Ss026jiLlJq3b8+A6ePwKA/S +JYh2VWAUyVusp7fusR+iI35m1D+JkdFrGuJqL3C/WlXZ/Ps97FRNhK/C0xVZ62v6 +sJSwrklvwVRdGjvWC7kHCIkFVopg5j2F5kyhBUZSRBsCAwEAAaOBkjCBjzASBgNV +HRMBAf8ECDAGAQH/AgEAMB0GA1UdDgQWBBRcWpT3rRGE0cgoYJjDD8pk0lVFKTBa +BgNVHSMEUzBRgBRc8vH0Uykjnu5AWpKjtCou9aaLZ6EvpC0wKzELMAkGA1UEBhMC +VVMxDTALBgNVBAMMBFRlc3QxDTALBgNVBAoMBFNhbHSCCG36YKj9FRj4MA0GCSqG +SIb3DQEBCwUAA4IBAQCprzp7z/NVZOXtZwW97LcJJLr9ukYb/rKLT+atTY2dFST+ +5TpMa1f89WoDPNcvSCJPeOXO9am9h43M9D47FE9X9q7HPO1OjW2ZP6ucPGJ8j3hR +VSxpDkc/g5jbWtPdx9RyUEsO/a34l+JPWgWXI+jz/PjE0ltqN6qhV71Q0DzDcw3D +JJ2QvEWCO1tti5L0crXOFCkEDnAXJqF603CVSvmymkcgGxT9kuaufbL0BWAV8pc8 +SDDbeH+eBxbZ/1rSfBGvW3mDDkL/wlz1LUC2u5n/w6xGxBr/ojONukiKIRwA3abB +8mWSEFTt7DfS0Uh6shcjzRM/dKEtox1qrtLZMr9O +-----END CERTIFICATE----- +""" + + +@pytest.fixture +def ca_sub_key(): + return """\ +-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDOtgdYqpfydnfV +euNl/1EFplR85J7Z1uMnWWvfGB8+WlUOv1WsKNrxz8SYTr7QaaJPqzRt2I+FTWxO +8A9MliPxWZfUl3t0J6hkZ1cAUdRFnJf4lSr55J8bbm/zPBcRPsI6Y3the6dHK7o8 +/ecv/48xGank1UW0xDEAmpVGgpEGEeweBS+AnbG3mZQMUPk9A0TFNSRud2mI1eT6 +xGlj+krNNuo4i5Sat2/PgOnj8CgP0iWIdlVgFMlbrKe37rEfoiN+ZtQ/iZHRaxri +ai9wv1pV2fz7PexUTYSvwtMVWetr+rCUsK5Jb8FUXRo71gu5BwiJBVaKYOY9heZM +oQVGUkQbAgMBAAECggEAMAFuK1VS/Ggu5FEpxmJI+rrqHCcsDQMutdC6kJEVkHGC +F26wAs9qKYZK7eQ7xEMEAuSLxIbqrdaRNLPjmbG0nzRjYmfbr9oV7VtihRx7476+ +PGjIFkjV+pTnQuHNqZ+dk9nOqZECBDFPiyKcMjVzl7+SCSbOjXCSwMUlrb5c17+e +eaLpEYRP0CgQhBPMzo8D2JUqERjHMwJtNbKN7vz6tSfSCL9kL4m7NkZcDPpK6A8E +bQb4ueCtecwUpCxBtSCyE3o9U1I4xIKePWiszSr7c430PUSXhAPjcDEY5N+AqfrB +abxPRg4Fo0KH+ZWPJ35FQF7v+hrTpGK2KmWt6Swf4QKBgQD8k39eOx/zl0pT+TPT +srM31eF9aITg0hZp1XDXJ/n7J7N+/OzgxYQQV3+UO9ksss34lCRViqNU4lLxhBIc +YcfXNk+yxMCoGhY5PJhPPPbkRmYWWU1RQ3s5V3tAdjDOK5kpUMZQxGHO6YW/RAJ/ +FiJmbRZcSHlsW2CkLVJzPAtWzwKBgQDRg14k5Kb2f7bq1CJyZsq6hhWCLt99JTG3 +9vl9Wga90KoeUonUe0hRaDw7kCp+uK+7Lu5lRw4pJW0dVqFX1HxSREAGANBhKMab +AN4mnQMvpJawJiGBSYxmUp565/NmKHBoTuugbGiCx0ftbzjtgGv85H3fzIVaRmuX +FAGqFBDQ9QKBgQDfyYQ5pqNJvguSWaPM93GJkEzJQ9kwJZTMUtw3FmmMWYHVix4K +jZbUr+IPIfPrgcWzcPa8gCj1Zc5dxUoSsaRSEAIPf/q/NtXoAsNkubx7R9DeDmPO +E79TcCp5U/8sPT7od3QvTcDnhssFS6n2llMGc7MzMte65T+8V5fNGC9nywKBgB6T ++sCNsqSVXUAGuARUZlA005zNdIbST+BWpnEaG5PGiZ2lVEJzv8lJ2kijMOCP2e4K +2nZjmXh94t/+TcwA0ig7l9CIe+FCT0I+LS4bimSAtBF/bzJsZpZkhobPpaGKU2WV +5yPhzpsPtLq9meRn8trVCl4Ifon/byJ8pAWLqiylAoGBAJct0hhpTIN+cUtY47cw +++E1tei8lyde61dV8ZHpIVhYoJ91qMAxQmIBXTJ1cE0HQ36hb5a4Np6VUVsjMovF +zk/kH99dgMUX7X8JI61EUlfM4FucgsUmleiIvJLy+/rn8RD24p9J9BDseKHkZ0C2 +JIjZyneR8x3D+yy95o7WS+Qo +-----END PRIVATE KEY----- +""" + + @pytest.fixture def rsa_privkey(): return """\ @@ -1053,6 +1121,35 @@ def test_certificate_managed_existing_chain(x509, cert_args): _assert_not_changed(ret) +@pytest.mark.parametrize("encoding", ("pem", "pkcs7_pem", "pkcs7_der", "pkcs12")) +def test_certificate_managed_multi_chain( + x509, + cert_args, + ca_cert, + ca_sub, + ca_sub_key, + rsa_privkey, + encoding, + ca_A, + ca_B, + ca_AI, + ca_BI, +): + """ + Ensure absence of order in pkcs7 certificates is accounted for. Also test the rest + with multiple appended certificates. + """ + cert_args["private_key"] = rsa_privkey + cert_args["encoding"] = encoding + cert_args["signing_cert"] = ca_sub + cert_args["signing_private_key"] = ca_sub_key + cert_args["append_certs"] = [ca_sub, ca_cert, ca_AI, ca_A, ca_BI, ca_B] + ret = x509.certificate_managed(**cert_args) + assert ret.result is True + ret = x509.certificate_managed(**cert_args) + _assert_not_changed(ret) + + @pytest.mark.usefixtures("existing_cert") @pytest.mark.parametrize( "existing_cert", diff --git a/tests/pytests/functional/utils/test_x509.py b/tests/pytests/functional/utils/test_x509.py index 1f9c8007901e..c9b33586363b 100644 --- a/tests/pytests/functional/utils/test_x509.py +++ b/tests/pytests/functional/utils/test_x509.py @@ -1,8 +1,10 @@ +import contextlib from base64 import b64decode from textwrap import dedent import pytest +import salt.modules.x509_v2 import salt.utils.x509 as x509 cx509 = pytest.importorskip("cryptography.x509") @@ -588,3 +590,190 @@ def test_load_cert_broken_pkcs7_pem(cert_pkcs7_pem): x509.CertDeserializationError, match="Could not load PEM-encoded PKCS.*" ): x509.load_cert(data) + + +@pytest.fixture +def ca_C(): + """ + self-signed root, did not issue ``leaf`` + """ + return """\ +-----BEGIN CERTIFICATE----- +MIIDODCCAiCgAwIBAgIIbfpgqP0VGPgwDQYJKoZIhvcNAQELBQAwKzELMAkGA1UE +BhMCVVMxDTALBgNVBAMMBFRlc3QxDTALBgNVBAoMBFNhbHQwHhcNMjIxMTE1MTQw +NDMzWhcNMzIxMTEyMTQwNDMzWjArMQswCQYDVQQGEwJVUzENMAsGA1UEAwwEVGVz +dDENMAsGA1UECgwEU2FsdDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB +AOGTScvrjcEt6vsJcG9RUp6fKaDNDWZnJET0omanK9ZwaoGpJPp8UDYe/8ADeI7N +10wdyB4oDM9gRDjInBtdQO/PsrmKZF6LzqVFgLMxu2up+PHMi9z6B2P4esIAzMu9 +PYxc9zH4HzLImHqscVD2HCabsjp9X134Af7hVY5NN/W/4qTP7uOM20wSG2TPI6+B +tA9VyPbEPMPRzXzrqc45rVYe6kb2bT84GE93Vcu/e5JZ/k2AKD8Hoa2cxLPsTLq5 +igl+D+k+dfUtiABiKPvVQiYBsD1fyHDn2m7B6pCgvrGqHjsoAKufgFnXy6PJRg7n +vQfaxSiusM5s+VS+fjlvgwsCAwEAAaNgMF4wDwYDVR0TBAgwBgEB/wIBATALBgNV +HQ8EBAMCAQYwHQYDVR0OBBYEFFzy8fRTKSOe7kBakqO0Ki71potnMB8GA1UdIwQY +MBaAFFzy8fRTKSOe7kBakqO0Ki71potnMA0GCSqGSIb3DQEBCwUAA4IBAQBZS4MP +fXYPoGZ66seM+0eikScZHirbRe8vHxHkujnTBUjQITKm86WeQgeBCD2pobgBGZtt +5YFozM4cERqY7/1BdemUxFvPmMFFznt0TM5w+DfGWVK8un6SYwHnmBbnkWgX4Srm +GsL0HHWxVXkGnFGFk6Sbo3vnN7CpkpQTWFqeQQ5rHOw91pt7KnNZwc6I3ZjrCUHJ ++UmKKrga16a4Q+8FBpYdphQU609npo/0zuaE6FyiJYlW3tG+mlbbNgzY/+eUaxt2 +9Bp9mtA+Hkox551Mfpq45Oi+ehwMt0xjZCjuFCM78oiUdHCGO+EmcT7ogiYALiOF +LN1w5sybsYwIw6QN +-----END CERTIFICATE----- +""" + + +@pytest.fixture +def ca_CI(): + """ + signed by ca_C + """ + return """\ +-----BEGIN CERTIFICATE----- +MIIDejCCAmKgAwIBAgIUYJfOr7sQ4QiGVO8/OOe+Jc9RL7cwDQYJKoZIhvcNAQEL +BQAwKzELMAkGA1UEBhMCVVMxDTALBgNVBAMMBFRlc3QxDTALBgNVBAoMBFNhbHQw +HhcNMjYwNzI4MDgwNjU1WhcNMzIxMTEyMTQwNDMzWjAuMQswCQYDVQQGEwJVUzEN +MAsGA1UECgwEU2FsdDEQMA4GA1UEAwwHVGVzdFN1YjCCASIwDQYJKoZIhvcNAQEB +BQADggEPADCCAQoCggEBAM62B1iql/J2d9V642X/UQWmVHzkntnW4ydZa98YHz5a +VQ6/Vawo2vHPxJhOvtBpok+rNG3Yj4VNbE7wD0yWI/FZl9SXe3QnqGRnVwBR1EWc +l/iVKvnknxtub/M8FxE+wjpje2F7p0crujz95y//jzEZqeTVRbTEMQCalUaCkQYR +7B4FL4CdsbeZlAxQ+T0DRMU1JG53aYjV5PrEaWP6Ss026jiLlJq3b8+A6ePwKA/S +JYh2VWAUyVusp7fusR+iI35m1D+JkdFrGuJqL3C/WlXZ/Ps97FRNhK/C0xVZ62v6 +sJSwrklvwVRdGjvWC7kHCIkFVopg5j2F5kyhBUZSRBsCAwEAAaOBkjCBjzASBgNV +HRMBAf8ECDAGAQH/AgEAMB0GA1UdDgQWBBRcWpT3rRGE0cgoYJjDD8pk0lVFKTBa +BgNVHSMEUzBRgBRc8vH0Uykjnu5AWpKjtCou9aaLZ6EvpC0wKzELMAkGA1UEBhMC +VVMxDTALBgNVBAMMBFRlc3QxDTALBgNVBAoMBFNhbHSCCG36YKj9FRj4MA0GCSqG +SIb3DQEBCwUAA4IBAQCprzp7z/NVZOXtZwW97LcJJLr9ukYb/rKLT+atTY2dFST+ +5TpMa1f89WoDPNcvSCJPeOXO9am9h43M9D47FE9X9q7HPO1OjW2ZP6ucPGJ8j3hR +VSxpDkc/g5jbWtPdx9RyUEsO/a34l+JPWgWXI+jz/PjE0ltqN6qhV71Q0DzDcw3D +JJ2QvEWCO1tti5L0crXOFCkEDnAXJqF603CVSvmymkcgGxT9kuaufbL0BWAV8pc8 +SDDbeH+eBxbZ/1rSfBGvW3mDDkL/wlz1LUC2u5n/w6xGxBr/ojONukiKIRwA3abB +8mWSEFTt7DfS0Uh6shcjzRM/dKEtox1qrtLZMr9O +-----END CERTIFICATE----- +""" + + +@pytest.fixture +def leaf(): + """ + Certificate issued by cross-signed intermediate + """ + return """\ +-----BEGIN CERTIFICATE----- +MIIBhDCCASqgAwIBAgIUUfho58FH0YibFO3PwI+GCCliPzswCgYIKoZIzj0EAwIw +FzEVMBMGA1UEAwwMSW50ZXJtZWRpYXRlMB4XDTI2MDYyODExNTE0MVoXDTI3MDcy +ODExNTE0MVowGzEZMBcGA1UEAwwQbGVhZi5leGFtcGxlLmNvbTBZMBMGByqGSM49 +AgEGCCqGSM49AwEHA0IABNlc5BexY+JmrhtMSp3y3KAF5X6ujudYihTh4/8tS4pZ +oB2HexRABBagDIuJDdktbYIH2Ryq60v7QnU+sHQO1JKjUDBOMAwGA1UdEwEB/wQC +MAAwHQYDVR0OBBYEFO7VPMpOeUsAA8G1BtTmSl/iwVEHMB8GA1UdIwQYMBaAFKZL +UKNDNkPUg8p5Z2Qm2ls3vnFyMAoGCCqGSM49BAMCA0gAMEUCIEECTpi6f06fNN1J +QUCEtf6lwCvapwyrvUDzJ0yqxD8jAiEAgTAVhMbRB5zsyxf3wh5Bq6woCUgQ72aD +1RZ5zi26gBs= +-----END CERTIFICATE----- +""" + + +@pytest.fixture +def ca_AI(): + """ + cross-signed intermediate, signed by ca_A + """ + return """\ +-----BEGIN CERTIFICATE----- +MIIBfjCCASOgAwIBAgIUYmpkbQuDqM3dO9i139Sx7ySHr/cwCgYIKoZIzj0EAwIw +ETEPMA0GA1UEAwwGUm9vdCBBMB4XDTI2MDYyODExNTE0MVoXDTI3MDcyODExNTE0 +MVowFzEVMBMGA1UEAwwMSW50ZXJtZWRpYXRlMFkwEwYHKoZIzj0CAQYIKoZIzj0D +AQcDQgAE9BftFUgrqiC09uL85Ywj+wW/u+x6RoUoUcgnsUYJoN2yIdXWGKsWoAU2 +jGfYoMcyOzxh2vANT96n1tICWUBk9aNTMFEwDwYDVR0TAQH/BAUwAwEB/zAdBgNV +HQ4EFgQUpktQo0M2Q9SDynlnZCbaWze+cXIwHwYDVR0jBBgwFoAUI5LLBbL2KL1u +R4izAau3IaAFWBIwCgYIKoZIzj0EAwIDSQAwRgIhAKWkHq8EBfUjyRrbgjzBrpfg +q7xQ68fvLWSF8Uh0iMLlAiEA+KyFHfCO6UoNL6A8z6+aMHoBoYmYLRHDSQyQz+nk +m7A= +-----END CERTIFICATE----- +""" + + +@pytest.fixture +def ca_A(): + """ + self-signed root + """ + return """\ +-----BEGIN CERTIFICATE----- +MIIBdzCCAR2gAwIBAgIUC/y/5CoGVmQZm94iCl2rvBzTQScwCgYIKoZIzj0EAwIw +ETEPMA0GA1UEAwwGUm9vdCBBMB4XDTI2MDYyODExNTE0MVoXDTI3MDcyODExNTE0 +MVowETEPMA0GA1UEAwwGUm9vdCBBMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE +4fc0YCVw4clsQbxr+o/N9nrHQXfB8fGjEYyOYGmLCkcGPrkDJhd0OURsWDiqxKRh +f5ZDSGZ2JXtMeMRJB3Rcj6NTMFEwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQU +I5LLBbL2KL1uR4izAau3IaAFWBIwHwYDVR0jBBgwFoAUI5LLBbL2KL1uR4izAau3 +IaAFWBIwCgYIKoZIzj0EAwIDSAAwRQIhANyDI1ej2lGw6boz41N77JTQY35fc30+ +gk7bkvfxIZopAiBZhQElEq8dbU73hwVrISQvM76IFNpQV5qW1Yiqlag8fA== +-----END CERTIFICATE----- +""" + + +@pytest.fixture +def ca_BI(): + """ + cross-signed intermediate, signed by ca_B + """ + return """\ +-----BEGIN CERTIFICATE----- +MIIBfTCCASOgAwIBAgIUD6smne5ycd6zLbfX63jMs+CPXOQwCgYIKoZIzj0EAwIw +ETEPMA0GA1UEAwwGUm9vdCBCMB4XDTI2MDYyODExNTE0MVoXDTI3MDcyODExNTE0 +MVowFzEVMBMGA1UEAwwMSW50ZXJtZWRpYXRlMFkwEwYHKoZIzj0CAQYIKoZIzj0D +AQcDQgAE9BftFUgrqiC09uL85Ywj+wW/u+x6RoUoUcgnsUYJoN2yIdXWGKsWoAU2 +jGfYoMcyOzxh2vANT96n1tICWUBk9aNTMFEwDwYDVR0TAQH/BAUwAwEB/zAdBgNV +HQ4EFgQUpktQo0M2Q9SDynlnZCbaWze+cXIwHwYDVR0jBBgwFoAUqEVIergleZMo +G5K+dKStHGLWGdgwCgYIKoZIzj0EAwIDSAAwRQIhAIYDBNXckhpStfcJkTr/+EnA +DUSXYcEE2hW2+fGFR93RAiAylTL28+jfb3wuSsl3dWgAVCiynPdbUYmaLHn/zuhs +oQ== +-----END CERTIFICATE----- +""" + + +@pytest.fixture +def ca_B(): + """ + self-signed root + """ + return """\ +-----BEGIN CERTIFICATE----- +MIIBdzCCAR2gAwIBAgIUWK37d5PTFxJNwJp2Ah78Uv99z94wCgYIKoZIzj0EAwIw +ETEPMA0GA1UEAwwGUm9vdCBCMB4XDTI2MDYyODExNTE0MVoXDTI3MDcyODExNTE0 +MVowETEPMA0GA1UEAwwGUm9vdCBCMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE +F4gYt8XU4zLjHsYycE4va8izSACMMs5A6k/KZDI+XdEV++A6Pp5hAyq/45LWqIQe +lwC/kAqcqY7Lbr3/U3XnzKNTMFEwDwYDVR0TAQH/BAUwAwEB/zAdBgNVHQ4EFgQU +qEVIergleZMoG5K+dKStHGLWGdgwHwYDVR0jBBgwFoAUqEVIergleZMoG5K+dKSt +HGLWGdgwCgYIKoZIzj0EAwIDSAAwRQIhAOSSlDW0gm5vYU9eljoeRARTbIyFvgO/ +ZOHdbEYHl5klAiBGHCe/f8phNPF/DOuKuqv81kGGmmvpGf7USwnZ+fdbjQ== +-----END CERTIFICATE----- +""" + + +@pytest.mark.parametrize("encoding", ("pkcs7_pem", "pkcs7_der")) +def test_load_cert_pkcs7_returns_correct_cert_and_chain(ca_A, ca_AI, leaf, encoding): + serialized = salt.modules.x509_v2.encode_certificate( + leaf, encoding=encoding, append_certs=[ca_A, ca_AI] + ) + cert, chain = x509.load_cert(serialized, load_chain=True) + cert_issuer = cert.issuer.rfc4514_string() + assert chain[0].subject.rfc4514_string() == cert_issuer + assert chain[1].subject.rfc4514_string() == chain[0].issuer.rfc4514_string() + + +@pytest.mark.parametrize("encoding", ("pkcs7_pem", "pkcs7_der")) +def test_load_cert_pkcs7_orders_chain_with_multiple_valid_paths( + encoding, ca_A, ca_AI, ca_B, ca_BI, leaf +): + serialized = salt.modules.x509_v2.encode_certificate( + leaf, encoding=encoding, append_certs=[ca_A, ca_AI, ca_B, ca_BI] + ) + cert, chain = x509.load_cert(serialized, load_chain=True) + assert cert.subject.rfc4514_string() == "CN=leaf.example.com" + assert len(chain) == 4 + cert_issuer = cert.issuer.rfc4514_string() + # Best case, we order the certificates by immediate certification, backwards, and account for cross-signing + assert chain[0].subject.rfc4514_string() == cert_issuer + assert chain[1].subject.rfc4514_string() == chain[0].issuer.rfc4514_string() + assert chain[1].subject.rfc4514_string() == chain[0].issuer.rfc4514_string() + orphan_subjects = {orphan.subject.rfc4514_string() for orphan in chain[2:]} + assert orphan_subjects == {"CN=Root A", "CN=Intermediate"} From 8cabca17251b53fefcf8cd64c6cf756bb2196013 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 28 Jul 2026 16:00:29 +0200 Subject: [PATCH 02/10] Account for PKCS#7 being unordered --- changelog/69893.fixed.md | 1 + salt/states/x509_v2.py | 15 ++- salt/utils/x509.py | 119 ++++++++++++++++++-- tests/pytests/functional/utils/test_x509.py | 55 +++++++++ 4 files changed, 177 insertions(+), 13 deletions(-) create mode 100644 changelog/69893.fixed.md diff --git a/changelog/69893.fixed.md b/changelog/69893.fixed.md new file mode 100644 index 000000000000..898d3455bbd6 --- /dev/null +++ b/changelog/69893.fixed.md @@ -0,0 +1 @@ +Fixed stateful management of PKCS#7 certificates with appended chain using `x509_v2.certificate_managed`. Also fixed loading of PKCS#7-encoded certificate bundles with `salt.utils.x509.load_cert`. diff --git a/salt/states/x509_v2.py b/salt/states/x509_v2.py index 1c2dc8edb590..b7a91fcba231 100644 --- a/salt/states/x509_v2.py +++ b/salt/states/x509_v2.py @@ -187,8 +187,11 @@ import os.path from datetime import datetime, timedelta, timezone +import salt.utils.dictupdate import salt.utils.files import salt.utils.platform +import salt.utils.stringutils +import salt.utils.versions from salt.exceptions import CommandExecutionError, SaltInvocationError from salt.state import STATE_INTERNAL_KEYWORDS as _STATE_INTERNAL_KEYWORDS @@ -505,7 +508,9 @@ def certificate_managed( current_chain = current_chain or [] ca_chain = [x509util.load_cert(x) for x in append_certs] - if not _compare_ca_chain(current_chain, ca_chain): + if not _compare_ca_chain( + current_chain, ca_chain, unordered="pkcs7" in current_encoding + ): changes["additional_certs"] = True ( @@ -1747,9 +1752,13 @@ def getextname(ext): return {"added": added, "changed": changed, "removed": removed} -def _compare_ca_chain(current, new): - if not len(current) == len(new): +def _compare_ca_chain(current, new, unordered=False): + if len(current) != len(new): return False + if unordered: + return {cert.fingerprint(hashes.SHA256()) for cert in new} == { + cert.fingerprint(hashes.SHA256()) for cert in current + } for i, new_cert in enumerate(new): if new_cert.fingerprint(hashes.SHA256()) != current[i].fingerprint( hashes.SHA256() diff --git a/salt/utils/x509.py b/salt/utils/x509.py index 19c83739b294..211a7c820d05 100644 --- a/salt/utils/x509.py +++ b/salt/utils/x509.py @@ -869,6 +869,98 @@ def load_pubkey(pk, get_encoding=False): raise PubDeserializationError("Could not load DER-encoded public key.") from err +def order_certs_naively(bundle, allow_orphans=True, require_leaf=True): + """ + Deterministically order certificates in a bundle using a naive algorithm. + This is not a chain building algorithm! It just selects the longest chain + of direct certification, preferring leaves by default, and appends all + orphans ordered by their fingerprints, if orphans are allowed. + + bundle + A set of cryptography.x509.Certificate objects to order. + + allow_orphans + Do not require all certificates to build a single chain. Defaults to true. + + require_leaf + Require that a path begins with a certificate that itself has not + been used to issue another certificate in the bundle. Defaults to true. + """ + if len(bundle) < 2: + return list(bundle) + + def _directly_issued_by(subject, issuer): + if subject.issuer != issuer.subject: + return False + try: + subject.verify_directly_issued_by(issuer) + except (InvalidSignature, TypeError, ValueError): + return False + return True + + def _fp(cert): + return cert.fingerprint(hashes.SHA256()) + + ordered_bundle = tuple(sorted(bundle, key=_fp)) + issuers = { + cert: [ + candidate + for candidate in ordered_bundle + if _directly_issued_by(cert, candidate) + ] + for cert in ordered_bundle + } + if require_leaf: + # ensure we treat self-signed root certificates that have not issued another certificate in this bundle as a leaf + cert_issuers = { + issuer + for subject, candidates in issuers.items() + for issuer in candidates + if issuer != subject + } + leaves = {cert for cert in ordered_bundle if cert not in cert_issuers} + if not leaves: + # This would be unusual, but possible when e.g. two certificates signed each other + raise ValueError( + "Certificate bundle did not contain a single leaf certificate" + ) + else: + leaves = {} + + def _paths_from( + cert, + seen, + ): + candidates = [issuer for issuer in issuers[cert] if issuer not in seen] + if not candidates: + return [[cert]] + return [ + [cert, *tail] + for issuer in candidates + for tail in _paths_from(issuer, seen | {issuer}) + ] + + paths = [ + path for cert in ordered_bundle for path in _paths_from(cert, frozenset({cert})) + ] + + # Longest path first; fingerprints provide a stable tie-breaker. + selected = min( + paths, + key=lambda path: ( + -int(path[0] in leaves), + -len(path), + tuple(_fp(cert) for cert in path), + ), + ) + orphans = [cert for cert in ordered_bundle if cert not in selected] + if not allow_orphans and orphans: + raise ValueError( + "Certificate bundle did not contain a singular chain comprising all certificates" + ) + return [*selected, *orphans] + + def load_cert(cert, passphrase=None, load_chain=False, get_encoding=False): """ Return a certificate instance from @@ -910,12 +1002,13 @@ def load_cert(cert, passphrase=None, load_chain=False, get_encoding=False): ) from err else: try: - loaded = pkcs7.load_pem_pkcs7_certificates(pems[0]) + chain = order_certs_naively(pkcs7.load_pem_pkcs7_certificates(pems[0])) + loaded = chain.pop(0) # the first cert is sure to be a leaf if load_chain: - return loaded.pop(0), loaded + return loaded, chain if get_encoding: - return loaded.pop(0), "pkcs7_pem", loaded, None - return loaded.pop(0) + return loaded, "pkcs7_pem", chain, None + return loaded except ValueError as err: raise CertDeserializationError( "Could not load PEM-encoded PKCS#7 blob" @@ -952,14 +1045,20 @@ def load_cert(cert, passphrase=None, load_chain=False, get_encoding=False): # PKCS7 try: # v37+ - loaded = pkcs7.load_der_pkcs7_certificates(cert) - if load_chain: - return loaded.pop(0), loaded - if get_encoding: - return loaded.pop(0), "pkcs7_der", loaded, None - return loaded[0] + bundle = pkcs7.load_der_pkcs7_certificates(cert) except ValueError: pass + else: + try: + chain = order_certs_naively(bundle) + except ValueError as err: + raise CertDeserializationError(str(err)) from err + loaded = chain.pop(0) # the first cert is sure to be a leaf + if load_chain: + return loaded, chain + if get_encoding: + return loaded, "pkcs7_der", chain, None + return loaded # nothing worked raise CertDeserializationError( "Could not deserialize binary data, neither as DER nor PKCS#7, PKCS#12." diff --git a/tests/pytests/functional/utils/test_x509.py b/tests/pytests/functional/utils/test_x509.py index c9b33586363b..756963a2e33f 100644 --- a/tests/pytests/functional/utils/test_x509.py +++ b/tests/pytests/functional/utils/test_x509.py @@ -777,3 +777,58 @@ def test_load_cert_pkcs7_orders_chain_with_multiple_valid_paths( assert chain[1].subject.rfc4514_string() == chain[0].issuer.rfc4514_string() orphan_subjects = {orphan.subject.rfc4514_string() for orphan in chain[2:]} assert orphan_subjects == {"CN=Root A", "CN=Intermediate"} + + +@pytest.mark.parametrize( + "certs,order", + ( + (["leaf"], ["leaf"]), + (["ca_A"], ["ca_A"]), + (["leaf", "ca_AI", "ca_A"], ["leaf", "ca_AI", "ca_A"]), + (["leaf", "ca_BI", "ca_B"], ["leaf", "ca_BI", "ca_B"]), + ( + ["ca_A", "ca_BI", "ca_B", "ca_AI", "leaf"], + ["leaf", "ca_BI", "ca_B", "ca_A", "ca_AI"], + ), + (["ca_A", "ca_B", "ca_C"], ["ca_C", "ca_B", "ca_A"]), + ( + ["ca_A", "ca_B", "ca_C", "leaf", "ca_AI", "ca_BI", "ca_CI"], + ["leaf", "ca_BI", "ca_B", "ca_C", "ca_CI", "ca_A", "ca_AI"], + ), + ), +) +def test_order_certs_naively_works(certs, order, request): + bundle = [x509.load_cert(request.getfixturevalue(cert)) for cert in certs] + ordered_bundle = [x509.load_cert(request.getfixturevalue(cert)) for cert in order] + res = x509.order_certs_naively(bundle) + assert res == ordered_bundle + + +@pytest.mark.parametrize( + "certs,expected", + ( + (["leaf"], ["leaf"]), + (["ca_A"], ["ca_A"]), + (["leaf", "ca_AI", "ca_A"], ["leaf", "ca_AI", "ca_A"]), + (["leaf", "ca_BI", "ca_B"], ["leaf", "ca_BI", "ca_B"]), + (["leaf", "ca_BI", "ca_B", "ca_AI"], False), + ( + ["ca_A", "ca_BI", "ca_B", "ca_AI", "leaf"], + False, + ), + (["ca_A", "ca_B", "ca_C"], False), + ), +) +def test_order_certs_naively_no_allow_orphans(certs, expected, request): + if expected is False: + ctx = pytest.raises(ValueError, match=".*did not contain a singular chain.*") + ordered_bundle = [] + else: + ordered_bundle = [ + x509.load_cert(request.getfixturevalue(cert)) for cert in expected + ] + ctx = contextlib.nullcontext() + bundle = [x509.load_cert(request.getfixturevalue(cert)) for cert in certs] + with ctx: + res = x509.order_certs_naively(bundle, allow_orphans=False) + assert res == ordered_bundle From 859782885bef762aa0e1fb3fcadd1b85bedca7e0 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 28 Jul 2026 16:38:59 +0200 Subject: [PATCH 03/10] Add test for issue #69895 --- .../pytests/functional/states/test_x509_v2.py | 70 +++++++++++++------ 1 file changed, 49 insertions(+), 21 deletions(-) diff --git a/tests/pytests/functional/states/test_x509_v2.py b/tests/pytests/functional/states/test_x509_v2.py index a1c2ef55af29..050862cbafd4 100644 --- a/tests/pytests/functional/states/test_x509_v2.py +++ b/tests/pytests/functional/states/test_x509_v2.py @@ -39,6 +39,11 @@ ] +@pytest.fixture(params=(False, True)) +def testmode(request): + return request.param + + @pytest.fixture(scope="module") def ca_dir(tmp_path_factory): ca_dir = tmp_path_factory.mktemp("ca") @@ -1549,13 +1554,17 @@ def test_certificate_managed_backup( @pytest.mark.parametrize( - "existing_symlink,existing_cert,encoding", - [("existing_cert", {}, "pem"), ("existing_cert", {"encoding": "der"}, "der")], + "existing_symlink,existing_cert,encoding,testmode", + [ + ("existing_cert", {}, "pem", False), + ("existing_cert", {}, "pem", True), + ("existing_cert", {"encoding": "der"}, "der", False), + ], indirect=["existing_symlink", "existing_cert"], ) @pytest.mark.parametrize("follow", [True, False]) def test_certificate_managed_follow_symlinks( - x509, cert_args, existing_symlink, follow, existing_cert, encoding + x509, cert_args, existing_symlink, follow, existing_cert, encoding, testmode ): """ file.managed follow_symlinks arg needs special attention as well since @@ -1563,10 +1572,12 @@ def test_certificate_managed_follow_symlinks( """ cert_args["name"] = str(existing_symlink) cert_args["encoding"] = encoding - assert pathlib.Path(cert_args["name"]).is_symlink() + syml = pathlib.Path(cert_args["name"]) + assert syml.is_symlink() cert_args["follow_symlinks"] = follow - ret = x509.certificate_managed(**cert_args) + ret = x509.certificate_managed(**cert_args, test=testmode) assert bool(ret.changes) == (not follow) + assert syml.is_symlink() is (testmode or follow) @pytest.mark.parametrize( @@ -1952,13 +1963,17 @@ def test_crl_managed_backup(x509, crl_args, ca_key, modules, backup, encoding): @pytest.mark.parametrize( - "existing_symlink,existing_crl,encoding", - [("existing_crl", {}, "pem"), ("existing_crl", {"encoding": "der"}, "der")], + "existing_symlink,existing_crl,encoding,testmode", + [ + ("existing_crl", {}, "pem", False), + ("existing_crl", {}, "pem", True), + ("existing_crl", {"encoding": "der"}, "der", False), + ], indirect=["existing_symlink", "existing_crl"], ) @pytest.mark.parametrize("follow", [True, False]) def test_crl_managed_follow_symlinks( - x509, crl_args, existing_symlink, follow, existing_crl, encoding + x509, crl_args, existing_symlink, follow, existing_crl, encoding, testmode ): """ file.managed follow_symlinks arg needs special attention as well since @@ -1966,10 +1981,12 @@ def test_crl_managed_follow_symlinks( """ crl_args["name"] = str(existing_symlink) crl_args["encoding"] = encoding - assert pathlib.Path(crl_args["name"]).is_symlink() + syml = pathlib.Path(crl_args["name"]) + assert syml.is_symlink() crl_args["follow_symlinks"] = follow - ret = x509.crl_managed(**crl_args) + ret = x509.crl_managed(**crl_args, test=testmode) assert bool(ret.changes) == (not follow) + assert syml.is_symlink() is (testmode or follow) @pytest.mark.parametrize( @@ -2240,25 +2257,30 @@ def test_csr_managed_backup(x509, csr_args, rsa_privkey, modules, backup, encodi @pytest.mark.parametrize( - "existing_symlink,existing_csr,encoding", - [("existing_csr", {}, "pem"), ("existing_csr", {"encoding": "der"}, "der")], + "existing_symlink,existing_csr,encoding,testmode", + [ + ("existing_csr", {}, "pem", False), + ("existing_csr", {}, "pem", True), + ("existing_csr", {"encoding": "der"}, "der", False), + ], indirect=["existing_symlink", "existing_csr"], ) @pytest.mark.parametrize("follow", [True, False]) def test_csr_managed_follow_symlinks( - x509, csr_args, existing_symlink, follow, existing_csr, encoding + x509, csr_args, existing_symlink, follow, existing_csr, encoding, testmode ): """ file.managed follow_symlinks arg needs special attention as well since the checking of the existing file is performed by the x509 module """ csr_args["name"] = str(existing_symlink) - assert pathlib.Path(csr_args["name"]).is_symlink() + syml = pathlib.Path(csr_args["name"]) + assert syml.is_symlink() csr_args["follow_symlinks"] = follow csr_args["encoding"] = encoding - ret = x509.csr_managed(**csr_args) + ret = x509.csr_managed(**csr_args, test=testmode) assert bool(ret.changes) == (not follow) - assert pathlib.Path(ret.name).is_symlink() == follow + assert syml.is_symlink() is (testmode or follow) @pytest.mark.parametrize( @@ -2548,13 +2570,17 @@ def test_private_key_managed_backup(x509, pk_args, modules, backup, encoding): @pytest.mark.parametrize( - "existing_symlink,existing_pk,encoding", - [("existing_pk", {}, "pem"), ("existing_pk", {"encoding": "der"}, "der")], + "existing_symlink,existing_pk,encoding,testmode", + [ + ("existing_pk", {}, "pem", False), + ("existing_pk", {}, "pem", True), + ("existing_pk", {"encoding": "der"}, "der", False), + ], indirect=["existing_symlink", "existing_pk"], ) @pytest.mark.parametrize("follow", [True, False]) def test_private_key_managed_follow_symlinks( - x509, pk_args, existing_symlink, follow, existing_pk, encoding + x509, pk_args, existing_symlink, follow, existing_pk, encoding, testmode ): """ file.managed follow_symlinks arg needs special attention as well since @@ -2562,10 +2588,12 @@ def test_private_key_managed_follow_symlinks( """ pk_args["name"] = str(existing_symlink) pk_args["encoding"] = encoding - assert pathlib.Path(pk_args["name"]).is_symlink() + syml = pathlib.Path(pk_args["name"]) + assert syml.is_symlink() pk_args["follow_symlinks"] = follow - ret = x509.private_key_managed(**pk_args) + ret = x509.private_key_managed(**pk_args, test=testmode) assert bool(ret.changes) == (not follow) + assert syml.is_symlink() is (testmode or follow) @pytest.mark.parametrize( From f50736686159bb024dd1caed27930ffeb7b3376c Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 28 Jul 2026 16:40:11 +0200 Subject: [PATCH 04/10] Don't delete symlinks in test mode --- changelog/69895.fixed.md | 1 + salt/states/x509_v2.py | 24 ++++++++++++++---------- 2 files changed, 15 insertions(+), 10 deletions(-) create mode 100644 changelog/69895.fixed.md diff --git a/changelog/69895.fixed.md b/changelog/69895.fixed.md new file mode 100644 index 000000000000..a5688a06a3ee --- /dev/null +++ b/changelog/69895.fixed.md @@ -0,0 +1 @@ +Fixed `x509_v2.certificate_managed` deleting symlinks in test mode if `follow_symlinks` was explicitly set to `false` diff --git a/salt/states/x509_v2.py b/salt/states/x509_v2.py index b7a91fcba231..31dcbd2a01ea 100644 --- a/salt/states/x509_v2.py +++ b/salt/states/x509_v2.py @@ -466,8 +466,9 @@ def certificate_managed( if file_args.get("follow_symlinks", True): real_name = os.path.realpath(name) else: - # workaround https://github.com/saltstack/salt/issues/31802 - __salt__["file.remove"](name) + if not __opts__["test"]: + # workaround https://github.com/saltstack/salt/issues/31802 + __salt__["file.remove"](name) replace = True if __salt__["file.file_exists"](real_name): @@ -871,8 +872,9 @@ def crl_managed( if file_args.get("follow_symlinks", True): real_name = os.path.realpath(name) else: - # workaround https://github.com/saltstack/salt/issues/31802 - __salt__["file.remove"](name) + if not __opts__["test"]: + # workaround https://github.com/saltstack/salt/issues/31802 + __salt__["file.remove"](name) replace = True if __salt__["file.file_exists"](real_name): @@ -1106,8 +1108,9 @@ def csr_managed( if file_args.get("follow_symlinks", True): real_name = os.path.realpath(name) else: - # workaround https://github.com/saltstack/salt/issues/31802 - __salt__["file.remove"](name) + if not __opts__["test"]: + # workaround https://github.com/saltstack/salt/issues/31802 + __salt__["file.remove"](name) replace = True if __salt__["file.file_exists"](real_name): @@ -1382,13 +1385,14 @@ def private_key_managed( if file_args.get("follow_symlinks", True): real_name = os.path.realpath(name) else: - # workaround https://github.com/saltstack/salt/issues/31802 - __salt__["file.remove"](name) + if not __opts__["test"]: + # workaround https://github.com/saltstack/salt/issues/31802 + __salt__["file.remove"](name) replace = True file_exists = __salt__["file.file_exists"](real_name) - if file_exists and not new: + if file_exists and not (new or replace): try: current, current_encoding, _ = x509util.load_privkey( real_name, passphrase=passphrase, get_encoding=True @@ -1445,7 +1449,7 @@ def private_key_managed( changes["keysize"] = check_keysize if encoding != current_encoding: changes["encoding"] = encoding - elif file_exists and new: + elif (file_exists and new) or replace: changes["replaced"] = name else: changes["created"] = name From 662f96bbeaff1e91307f8d09143b67793a61767f Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 28 Jul 2026 19:08:55 +0200 Subject: [PATCH 05/10] Minor doc fixes --- salt/modules/x509_v2.py | 5 ++--- salt/states/x509_v2.py | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/salt/modules/x509_v2.py b/salt/modules/x509_v2.py index ea91c79bc08c..55def98ad8d1 100644 --- a/salt/modules/x509_v2.py +++ b/salt/modules/x509_v2.py @@ -236,7 +236,7 @@ def create_certificate( .. note:: - Mind that when ``der`` encoding is in use, appending certificatees is prohibited. + Mind that when ``der`` encoding is in use, appending certificates is prohibited. copypath Create a copy of the issued certificate in PEM format in this directory. @@ -681,7 +681,7 @@ def encode_certificate( .. note:: - Mind that when ``der`` encoding is in use, appending certificatees is prohibited. + Mind that when ``der`` encoding is in use, appending certificates is prohibited. private_key For ``pkcs12``, the private key corresponding to the public key of the ``certificate`` @@ -2059,7 +2059,6 @@ def verify_signature( certificate. signing_pub_key_passphrase - If ``signing_pub_key`` is encrypted, the passphrase to decrypt it. """ cert = x509util.load_cert(certificate) diff --git a/salt/states/x509_v2.py b/salt/states/x509_v2.py index 31dcbd2a01ea..c52e9113751d 100644 --- a/salt/states/x509_v2.py +++ b/salt/states/x509_v2.py @@ -285,7 +285,7 @@ def certificate_managed( .. note:: - Mind that when ``der`` encoding is in use, appending certificatees is prohibited. + Mind that when ``der`` encoding is in use, appending certificates is prohibited. copypath Create a copy of the issued certificate in PEM format in this directory. @@ -1052,7 +1052,7 @@ def csr_managed( Ignored for ``ed25519`` and ``ed448`` key types. encoding - Specify the encoding of the resulting certificate revocation list. + Specify the encoding of the resulting certificate signing request. It can be serialized as a ``pem`` text or binary ``der`` file. Defaults to ``pem``. From 863c1d13fbec9f46abf0655fdd29c9654ec5475e Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 28 Jul 2026 19:19:39 +0200 Subject: [PATCH 06/10] Add test for issue #69896 --- tests/pytests/functional/states/test_x509_v2.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/pytests/functional/states/test_x509_v2.py b/tests/pytests/functional/states/test_x509_v2.py index 050862cbafd4..a5ada53daab6 100644 --- a/tests/pytests/functional/states/test_x509_v2.py +++ b/tests/pytests/functional/states/test_x509_v2.py @@ -1689,6 +1689,13 @@ def test_crl_managed_exts(x509, crl_args, crl_args_exts, ca_key): assert len(crl.extensions) == len(crl_args_exts) +def test_crl_managed_no_signing_cert(x509, crl_args): + crl_args.pop("signing_cert") + ret = x509.crl_managed(**crl_args) + assert ret.result is False + assert "`signing_cert`" in ret.comment + + def test_crl_managed_test_true(x509, crl_args, crl_revoked): crl_args["revoked"] = crl_revoked crl_args["test"] = True From ed5fdca2a6ef03d349844384edebd7e12bd4bef8 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 28 Jul 2026 19:21:32 +0200 Subject: [PATCH 07/10] Require `signing_cert` --- changelog/69896.fixed.md | 1 + salt/modules/x509_v2.py | 4 ++-- salt/states/x509_v2.py | 12 +++++++++--- salt/utils/x509.py | 12 +++++------- 4 files changed, 17 insertions(+), 12 deletions(-) create mode 100644 changelog/69896.fixed.md diff --git a/changelog/69896.fixed.md b/changelog/69896.fixed.md new file mode 100644 index 000000000000..00f844c21f8c --- /dev/null +++ b/changelog/69896.fixed.md @@ -0,0 +1 @@ +Fixed traceback when `signing_cert` was not passed to `x509_v2.crl_managed` or `x509_v2.create_crl`. It has always been required. diff --git a/salt/modules/x509_v2.py b/salt/modules/x509_v2.py index 55def98ad8d1..9df431a2a50f 100644 --- a/salt/modules/x509_v2.py +++ b/salt/modules/x509_v2.py @@ -794,7 +794,7 @@ def encode_certificate( def create_crl( signing_private_key, revoked, - signing_cert=None, + signing_cert, signing_private_key_passphrase=None, include_expired=False, days_valid=None, @@ -856,7 +856,7 @@ def create_crl( The value should be a string in the same format as ``revocation_date``. signing_cert - The CA certificate to be used for signing the CRL. + The CA certificate to be used for signing the CRL. Required. signing_private_key_passphrase If ``signing_private_key`` is encrypted, the passphrase to decrypt it. diff --git a/salt/states/x509_v2.py b/salt/states/x509_v2.py index c52e9113751d..3ab4b7fb3ec7 100644 --- a/salt/states/x509_v2.py +++ b/salt/states/x509_v2.py @@ -736,6 +736,7 @@ def crl_managed( signing_cert The CA certificate to be used for signing the issued certificate. + Required. signing_private_key_passphrase If ``signing_private_key`` is encrypted, the passphrase to decrypt it. @@ -840,15 +841,20 @@ def crl_managed( "result": True, "comment": "The certificate revocation list is in the correct state", } - current = current_encoding = None + current = None changes = {} verb = "create" file_args, extra_args = _split_file_kwargs(_filter_state_internal_kwargs(kwargs)) extensions = extensions or {} - if extra_args: - raise SaltInvocationError(f"Unrecognized keyword arguments: {list(extra_args)}") try: + if extra_args: + raise SaltInvocationError( + f"Unrecognized keyword arguments: {list(extra_args)}" + ) + if not signing_cert: + raise SaltInvocationError("`signing_cert` is required") + # check file.managed changes early to avoid using unnecessary resources file_managed_test = _file_managed(name, test=True, replace=False, **file_args) diff --git a/salt/utils/x509.py b/salt/utils/x509.py index 211a7c820d05..3b9f58ce71b0 100644 --- a/salt/utils/x509.py +++ b/salt/utils/x509.py @@ -475,7 +475,7 @@ def build_csr(private_key, private_key_passphrase=None, subject=None, **kwargs): def build_crl( signing_private_key, revoked, - signing_cert=None, + signing_cert, signing_private_key_passphrase=None, include_expired=False, days_valid=100, @@ -488,24 +488,22 @@ def build_crl( Also returns signing private key. """ extensions = extensions or {} - if signing_cert: - signing_cert = load_cert(signing_cert) + signing_cert = load_cert(signing_cert) signing_private_key = load_privkey( signing_private_key, passphrase=signing_private_key_passphrase ) - if signing_cert and not is_pair(signing_cert.public_key(), signing_private_key): + if not is_pair(signing_cert.public_key(), signing_private_key): raise SaltInvocationError( "Signing private key does not match the certificate's public key" ) builder = cx509.CertificateRevocationListBuilder() - if signing_cert: - builder = builder.issuer_name(signing_cert.subject) + builder = builder.issuer_name(signing_cert.subject) builder = builder.last_update(datetime.now(tz=timezone.utc)) builder = builder.next_update( datetime.now(tz=timezone.utc) + timedelta(days=days_valid) ) for rev in revoked: - serial_number = not_after = revocation_date = None + serial_number = not_after = None if "not_after" in rev: not_after = datetime.strptime(rev["not_after"], TIME_FMT).replace( tzinfo=timezone.utc From 8feb73358280ca17031747df4d2076514e16f512 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 28 Jul 2026 22:17:26 +0200 Subject: [PATCH 08/10] Add tests for issue #69898 --- .../functional/modules/test_x509_v2.py | 3 +- .../pytests/functional/states/test_x509_v2.py | 22 ++++++++++ tests/pytests/unit/utils/test_x509.py | 41 +++++++++++++++---- 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/tests/pytests/functional/modules/test_x509_v2.py b/tests/pytests/functional/modules/test_x509_v2.py index 7ff0f7fda92a..82ea650199e8 100644 --- a/tests/pytests/functional/modules/test_x509_v2.py +++ b/tests/pytests/functional/modules/test_x509_v2.py @@ -1441,7 +1441,8 @@ def test_create_private_key_pkcs12(x509, passphrase): @pytest.mark.parametrize("encoding", ["pem", "der"]) def test_create_private_key_write_to_path(x509, encoding, tmp_path): tgt = tmp_path / "pk" - x509.create_private_key(encoding=encoding, path=str(tgt)) + res = x509.create_private_key(encoding=encoding, path=str(tgt)) + assert str(tgt) in res assert tgt.exists() if encoding == "pem": assert tgt.read_text().startswith("-----BEGIN PRIVATE KEY-----") diff --git a/tests/pytests/functional/states/test_x509_v2.py b/tests/pytests/functional/states/test_x509_v2.py index a5ada53daab6..f420a0fd5c95 100644 --- a/tests/pytests/functional/states/test_x509_v2.py +++ b/tests/pytests/functional/states/test_x509_v2.py @@ -1888,6 +1888,28 @@ def test_crl_managed_existing_encoding_change_only(x509, crl_args, ca_key): assert new.extensions[0].value.crl_number == 1 +def test_crl_managed_existing_revocation_extension_added(x509, crl_args, ca_key): + crl_args["revoked"] = [{"serial_number": "01337A"}] + ret = x509.crl_managed(**crl_args) + _assert_crl_basic(ret, ca_key) + crl_args["revoked"] = [ + {"serial_number": "01337A", "extensions": {"CRLReason": "keyCompromise"}} + ] + ret = x509.crl_managed(**crl_args) + _assert_crl_basic(ret, ca_key) + assert "revocations" in ret.changes + assert len(ret.changes["revocations"]["changed"]) == 1 + + +@pytest.mark.usefixtures("existing_crl") +def test_crl_managed_existing_crlnumber_auto_added(x509, crl_args, ca_key): + crl_args["extensions"] = {"cRLNumber": "auto"} + ret = x509.crl_managed(**crl_args) + assert ret.result is True + new = _get_crl(crl_args["name"]) + assert new.extensions.get_extension_for_class(cx509.CRLNumber).value.crl_number == 1 + + @pytest.mark.skip_on_windows @pytest.mark.parametrize("mode", ["0400", "0640", "0644"]) def test_crl_managed_mode(x509, crl_args, ca_key, mode, modules): diff --git a/tests/pytests/unit/utils/test_x509.py b/tests/pytests/unit/utils/test_x509.py index 7776257137a3..5b8113a08e1e 100644 --- a/tests/pytests/unit/utils/test_x509.py +++ b/tests/pytests/unit/utils/test_x509.py @@ -518,6 +518,7 @@ def test_create_issuer_alt_name( ), ( [ + "critical", {"OCSP": "URI:http://ocsp.example.com/"}, {"OCSP": "URI:http://ocsp2.example.com/"}, ], @@ -1577,7 +1578,7 @@ def test_get_dn(inpt, expected): ), { "critical": False, - "value": ["mail:ca@example.com", "DNS:example.com", "DNS:example.io"], + "value": ["email:ca@example.com", "DNS:example.com", "DNS:example.io"], }, ), ( @@ -1594,7 +1595,7 @@ def test_get_dn(inpt, expected): ), { "critical": False, - "value": ["mail:ca@example.com", "DNS:example.com", "DNS:example.io"], + "value": ["email:ca@example.com", "DNS:example.com", "DNS:example.io"], }, ), ( @@ -1634,7 +1635,7 @@ def test_get_dn(inpt, expected): ), critical=False, ), - {"critical": False, "value": ["DNS:example.io", "mail:hello@example.io"]}, + {"critical": False, "value": ["DNS:example.io", "email:hello@example.io"]}, ), ( cx509.Extension( @@ -1763,7 +1764,7 @@ def test_get_dn(inpt, expected): "onlyAA": False, "onlyCA": False, "onlyuser": True, - "onysomereasons": ["keyCompromise"], + "onlysomereasons": ["keyCompromise"], "relativename": None, }, ), @@ -1799,7 +1800,7 @@ def test_get_dn(inpt, expected): { "explicit_text": "mytext", "notice_numbers": [1, 2, 3], - "organizataion": "myorg", + "organization": "myorg", }, ] } @@ -1838,8 +1839,8 @@ def test_get_dn(inpt, expected): ), { "critical": False, - "excluded": ["mail:.com"], - "permitted": ["IP:192.168.0.0/16", "mail:.example.com"], + "excluded": ["email:.com"], + "permitted": ["IP:192.168.0.0/16", "email:.example.com"], }, ), ( @@ -1976,3 +1977,29 @@ def dtn(tz=None): assert crl.last_update_utc == curr_time_utc except AttributeError: assert crl.last_update == curr_time_utc_naive + + +@pytest.mark.parametrize("timestr", ("not_before", "not_after")) +def test_build_crt_malformed_date_raises_salt_invocation_error(ca_key, timestr): + """ + A malformed not_before/not_after must surface as a SaltInvocationError + (caught by the state) instead of a raw ValueError from strptime. + """ + with pytest.raises( + salt.exceptions.SaltInvocationError, match=f"Invalid date.*{timestr}.*" + ): + x509.build_crt(ca_key, **{timestr: "booh"}) + + +@pytest.mark.parametrize("timestr", ("not_after", "revocation_date")) +def test_build_crl_malformed_date_raises_salt_invocation_error( + ca_cert, ca_key, timestr +): + """ + Malformed date definitions must surface as a SaltInvocationError + (caught by the state) instead of a raw ValueError from strptime. + """ + with pytest.raises( + salt.exceptions.SaltInvocationError, match=f"Invalid date.*{timestr}.*" + ): + x509.build_crl(ca_key, [{"serial_number": 1, timestr: "booh"}], ca_cert) From 2ebfcdfa68cb76b2983ae7e0434141a33479c2f7 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Tue, 28 Jul 2026 22:20:57 +0200 Subject: [PATCH 09/10] Fix tracebacks and output inconsistencies --- changelog/69898.fixed.md | 1 + salt/modules/x509_v2.py | 3 +- salt/states/x509_v2.py | 39 ++++++++------- salt/utils/x509.py | 50 +++++++++---------- .../functional/modules/test_x509_v2.py | 8 +-- 5 files changed, 51 insertions(+), 50 deletions(-) create mode 100644 changelog/69898.fixed.md diff --git a/changelog/69898.fixed.md b/changelog/69898.fixed.md new file mode 100644 index 000000000000..229f18a59492 --- /dev/null +++ b/changelog/69898.fixed.md @@ -0,0 +1 @@ +Fixed some tracebacks being thrown instead of errors being reported in `x509_v2`. Fixed a typo in the rendered output of `issuingDistributionPoint` and `certificatePolicies` extensions. Fixed rendered prefix of an `RFC822Name`. diff --git a/salt/modules/x509_v2.py b/salt/modules/x509_v2.py index 9df431a2a50f..95a38079c9fb 100644 --- a/salt/modules/x509_v2.py +++ b/salt/modules/x509_v2.py @@ -165,6 +165,7 @@ import salt.utils.dictupdate import salt.utils.files import salt.utils.stringutils +import salt.utils.versions from salt.exceptions import CommandExecutionError, SaltInvocationError log = logging.getLogger(__name__) @@ -1294,7 +1295,7 @@ def create_private_key( ) with salt.utils.files.fopen(path, "wb") as fp_: fp_.write(out) - return + return f"File written to {path}" def encode_private_key( diff --git a/salt/states/x509_v2.py b/salt/states/x509_v2.py index 3ab4b7fb3ec7..dc1547e79723 100644 --- a/salt/states/x509_v2.py +++ b/salt/states/x509_v2.py @@ -938,11 +938,15 @@ def crl_managed( if crl_auto: # put cRLNumber = auto back if it was set extensions["cRLNumber"] = "auto" - changes["extensions"]["removed"].pop( - changes["extensions"]["removed"].index("cRLNumber") - ) - if not any(changes["extensions"].values()): - changes.pop("extensions") + try: + changes["extensions"]["removed"].remove("cRLNumber") + if not any(changes["extensions"].values()): + changes.pop("extensions") + except (KeyError, ValueError): + # cRLNumber was added to an existing CRL + changes.setdefault("extensions", {}).setdefault( + "added", [] + ).append("cRLNumber") else: changes["created"] = name @@ -1656,7 +1660,6 @@ def _compare_cert(current, builder, signing_cert, serial_number, not_before, not def _compare_csr(current, builder): changes = {} - # if _getattr_safe(builder, "_subject_name") != current.subject: if not _compareattr_safe(builder, "_subject_name", current.subject): changes["subject_name"] = _getattr_safe( builder, "_subject_name" @@ -1689,31 +1692,29 @@ def _get_extension_for_oid(extensions, oid): if not current.is_signature_valid(sig_pubkey): changes["public_key"] = True - rev_changes = {"added": [], "changed": [], "removed": []} + rev_changes = {"added": set(), "changed": set(), "removed": set()} revoked = _getattr_safe(builder, "_revoked_certificates") for rev in revoked: cur = current.get_revoked_certificate_by_serial_number(rev.serial_number) if cur is None: # certificate was not revoked before - rev_changes["added"].append(x509util.dec2hex(rev.serial_number)) + rev_changes["added"].add(x509util.dec2hex(rev.serial_number)) continue for ext in rev.extensions: cur_ext = _get_extension_for_oid(cur.extensions, ext.oid) # revoked certificate's extensions have changed (added/changed) - if any( - ( - cur_ext is None, - cur_ext.critical != ext.critical, - cur_ext.value != ext.value, - ) + if ( + cur_ext is None + or cur_ext.critical != ext.critical + or cur_ext.value != ext.value ): - rev_changes["changed"].append(x509util.dec2hex(rev.serial_number)) + rev_changes["changed"].add(x509util.dec2hex(rev.serial_number)) for cur_ext in cur.extensions: if _get_extension_for_oid(rev.extensions, cur_ext.oid) is None: # an extension was removed from from the revoked certificate - rev_changes["changed"].append(x509util.dec2hex(rev.serial_number)) + rev_changes["changed"].add(x509util.dec2hex(rev.serial_number)) for rev in current: # certificate was removed from the CRL, probably because it was outdated anyways @@ -1721,10 +1722,12 @@ def _get_extension_for_oid(extensions, oid): _get_revoked_certificate_by_serial_number(revoked, rev.serial_number) is None ): - rev_changes["removed"].append(x509util.dec2hex(rev.serial_number)) + rev_changes["removed"].add(x509util.dec2hex(rev.serial_number)) if any(rev_changes.values()): - changes["revocations"] = rev_changes + changes["revocations"] = { + typ: list(sorted(val)) for typ, val in rev_changes.items() + } ext_changes = _compare_exts(current, builder) if any(ext_changes.values()): diff --git a/salt/utils/x509.py b/salt/utils/x509.py index 3b9f58ce71b0..c6bf34604a4a 100644 --- a/salt/utils/x509.py +++ b/salt/utils/x509.py @@ -390,15 +390,9 @@ def build_crt( signing_cert.subject if not self_signed else subject_name ) - not_before = ( - datetime.strptime(not_before, TIME_FMT).replace(tzinfo=timezone.utc) - if not_before - else datetime.now(tz=timezone.utc) - ) - not_after = ( - datetime.strptime(not_after, TIME_FMT).replace(tzinfo=timezone.utc) - if not_after - else datetime.now(tz=timezone.utc) + timedelta(days=days_valid) + not_before = _strptime(not_before, "not_before") or datetime.now(tz=timezone.utc) + not_after = _strptime(not_after, "not_after") or ( + datetime.now(tz=timezone.utc) + timedelta(days=days_valid) ) builder = builder.not_valid_before(not_before).not_valid_after(not_after) @@ -503,13 +497,8 @@ def build_crl( datetime.now(tz=timezone.utc) + timedelta(days=days_valid) ) for rev in revoked: - serial_number = not_after = None - if "not_after" in rev: - not_after = datetime.strptime(rev["not_after"], TIME_FMT).replace( - tzinfo=timezone.utc - ) - if "serial_number" in rev: - serial_number = rev["serial_number"] + serial_number = rev.get("serial_number") + not_after = _strptime(rev.get("not_after"), "not_after") if "certificate" in rev: rev_cert = load_cert(rev["certificate"]) serial_number = rev_cert.serial_number @@ -524,13 +513,9 @@ def build_crl( if not_after and not include_expired: if datetime.now(tz=timezone.utc) > not_after: continue - if "revocation_date" in rev: - revocation_date = datetime.strptime( - rev["revocation_date"], TIME_FMT - ).replace(tzinfo=timezone.utc) - else: - revocation_date = datetime.now(tz=timezone.utc) - + revocation_date = _strptime( + rev.get("revocation_date"), "revocation_date" + ) or datetime.now(tz=timezone.utc) revoked_cert = cx509.RevokedCertificateBuilder( serial_number=serial_number, revocation_date=revocation_date ) @@ -1471,7 +1456,7 @@ def _create_authority_info_access(val, **kwargs): elif isinstance(val, dict): val = ((k, v) for k, v in val.items() if k != "critical") elif isinstance(val, list): - val = ((k, v) for x in val for k, v in x.items() if x != "critical") + val = ((k, v) for x in val if x != "critical" for k, v in x.items()) parsed = [] for oid, general_name in val: @@ -2077,7 +2062,7 @@ def render_gn(gn): if isinstance(gn, cx509.IPAddress): return f"IP:{gn.value.exploded}" if isinstance(gn, cx509.RFC822Name): - return f"mail:{gn.value}" + return f"email:{gn.value}" if isinstance(gn, cx509.RegisteredID): return f"RID:{gn.value.dotted_string}" if isinstance(gn, cx509.UniformResourceIdentifier): @@ -2216,7 +2201,7 @@ def _render_distribution_points(ext): def _render_issuing_distribution_point(ext): return { "fullname": [render_gn(x) for x in ext.value.full_name or []], - "onysomereasons": list( + "onlysomereasons": list( sorted(x.value for x in ext.value.only_some_reasons or []) ), "relativename": ( @@ -2249,7 +2234,7 @@ def _render_certificate_policies(ext): notice_numbers = notice.notice_reference.notice_numbers qualifiers.append( { - "organizataion": organization, + "organization": organization, "notice_numbers": notice_numbers, "explicit_text": notice.explicit_text, } @@ -2306,6 +2291,17 @@ def _render_invalidity_date(ext): return {"value": ext.value.invalidity_date.strftime(TIME_FMT)} +def _strptime(val, param): + if val is None: + return val + try: + return datetime.strptime(val, TIME_FMT).replace(tzinfo=timezone.utc) + except ValueError as err: + raise SaltInvocationError( + f"Invalid date format in param `{param}`: {err}" + ) from err + + EXTENSION_RENDERERS = immutabletypes.freeze( { cx509.BasicConstraints: _render_basic_constraints, diff --git a/tests/pytests/functional/modules/test_x509_v2.py b/tests/pytests/functional/modules/test_x509_v2.py index 82ea650199e8..e51001b4b6ad 100644 --- a/tests/pytests/functional/modules/test_x509_v2.py +++ b/tests/pytests/functional/modules/test_x509_v2.py @@ -422,7 +422,7 @@ def cert_exts_read(): }, "nameConstraints": { "critical": False, - "excluded": ["mail:.com"], + "excluded": ["email:.com"], "permitted": ["IP:192.168.0.0/16"], }, "noCheck": {"critical": False, "value": True}, @@ -433,7 +433,7 @@ def cert_exts_read(): }, "subjectAltName": { "critical": False, - "value": ["DNS:sub.salt.ca", "mail:sub@salt.ca"], + "value": ["DNS:sub.salt.ca", "email:sub@salt.ca"], }, "subjectKeyIdentifier": { "critical": False, @@ -509,7 +509,7 @@ def csr_exts_read(): }, "nameConstraints": { "critical": False, - "excluded": ["mail:.com"], + "excluded": ["email:.com"], "permitted": ["IP:192.168.0.0/16"], }, "noCheck": {"critical": False, "value": True}, @@ -520,7 +520,7 @@ def csr_exts_read(): }, "subjectAltName": { "critical": False, - "value": ["DNS:sub.salt.ca", "mail:sub@salt.ca"], + "value": ["DNS:sub.salt.ca", "email:sub@salt.ca"], }, "subjectKeyIdentifier": { "critical": False, From 7765af7062151374e0cc8796b3abdb6c144f532a Mon Sep 17 00:00:00 2001 From: jeanluc Date: Wed, 29 Jul 2026 04:52:03 +0200 Subject: [PATCH 10/10] Support `otherName` in `x509_v2` --- changelog/69900.fixed.md | 1 + salt/modules/x509_v2.py | 35 ++++- salt/utils/x509.py | 105 +++++++++++++- .../functional/modules/test_x509_v2.py | 2 +- .../pytests/functional/states/test_x509_v2.py | 6 +- tests/pytests/unit/utils/test_x509.py | 128 +++++++++++++++++- 6 files changed, 265 insertions(+), 12 deletions(-) create mode 100644 changelog/69900.fixed.md diff --git a/changelog/69900.fixed.md b/changelog/69900.fixed.md new file mode 100644 index 000000000000..7efa6c6c19e2 --- /dev/null +++ b/changelog/69900.fixed.md @@ -0,0 +1 @@ +Added support for `otherName` definitions in `x509_v2`, e.g. inside a `subjectAltNames` extension. diff --git a/salt/modules/x509_v2.py b/salt/modules/x509_v2.py index 95a38079c9fb..4b204561b20c 100644 --- a/salt/modules/x509_v2.py +++ b/salt/modules/x509_v2.py @@ -403,7 +403,7 @@ def create_certificate( ``keyid:always, issuer`` subjectAltName - There is support for all OpenSSL-defined types except ``otherName``. + There is support for all OpenSSL-defined types, but ``otherName`` support is limited. ``email:me@example.com,DNS:example.com`` or @@ -412,6 +412,39 @@ def create_certificate( - subjectAltName: - email:me@example.com # list items can be strings - dns: example.com # or single-key dicts + - ip: 1.2.3.4 + - otherName: + oid: 1.2.3.4.5.5 + value: some utf8 string + - otherName: + oid: 1.2.3.4.5.6 + value: true # this renders a BOOL:TRUE + - otherName: + oid: 1.2.3.4.5.7.7 + der: "hex:0101ff" # raw DER passthrough, hex-encoded + - otherName: + oid: 1.2.3.4.5.7.7 + der: "b64:AQH/" # raw DER passthrough, base64-encoded + - dirName: + C: US + ST: California + L: San Francisco + O: My Company + CN: mysite.com + + .. versionchanged:: 3006.28 + + ``otherName`` support was added. + + .. note:: + + Regarding ``otherName`` support: + + * OpenSSL-style strings (``otherName:1.2.3.4;UTF8:foo``) only allow ``UTF8`` type data. + * Dictionary definitions can additionally render other simple types like booleans by passing + in a value of the type. + * Arbitrary DER is supported by passing it in ``der``, with either ``hex:`` (hexadecimal encoding) + or ``b64:`` (base64 encoding) prefix. issuerAltName The syntax is the same as for ``subjectAltName``, except that the additional diff --git a/salt/utils/x509.py b/salt/utils/x509.py index c6bf34604a4a..54dd893d39ea 100644 --- a/salt/utils/x509.py +++ b/salt/utils/x509.py @@ -12,6 +12,7 @@ import cryptography from cryptography import x509 as cx509 from cryptography.exceptions import InvalidSignature +from cryptography.hazmat import asn1 from cryptography.hazmat.primitives import hashes, serialization from cryptography.hazmat.primitives.asymmetric import ec, ed448, ed25519, padding, rsa from cryptography.hazmat.primitives.serialization import pkcs7, pkcs12 @@ -1851,6 +1852,100 @@ def _deserialize_openssl_confstring(conf, multiple=False): }, critical +def _parse_other_name(value): + """ + Parse otherName definition. Accepted formats: + + OpenSSL-style string + e.g. ``1.2.3.4;UTF8:foobar``. Can only map to UTF8STRING, other ASN1 types raise an exception. + + Dictionary + ``{oid: 1.2.3.4, value: foobar}``: ``value`` is passed into the encoder, + meaning other simple types (in addition to UTF8, like BOOLEAN) are supported, even from SLS files. + In theory, more complex types can be passed in programmatically from Python. + + ``{oid: 1.2.3.4, der: "hex:deadbeef"}``: ``der`` can be an arbitrary DER blob. + It needs to be a hex/base64-encoded string with ``hex:``/``b64:`` prefix. + Raw Python bytes are passed through. + """ + if isinstance(value, str): + try: + oid_text, asn_expr = value.split(";", maxsplit=1) + except ValueError as err: + raise SaltInvocationError( + "`othername` string definition needs semicolon (;) between OID and " + "value: othername:1.2.3.4;UTF8:value" + ) from err + asn_expr = asn_expr.removeprefix( + "FORMAT:UTF8," + ) # Compatibility with OpenSSL's documented SmtpUTF8Mailbox spelling + try: + asn_typ, asn_val = asn_expr.split(":", maxsplit=1) + except ValueError as err: + raise SaltInvocationError( + "`othername` string definition needs colon (:) between value type and " + "value: othername:1.2.3.4;UTF8:value" + ) from err + if asn_typ.upper() not in {"UTF8", "UTF8STRING"}: + raise SaltInvocationError( + f"Unsupported otherName ASN.1 type {asn_typ!r}; only UTF8STRING is supported" + ) + oid = _get_oid(oid_text) + try: + encoded = asn1.encode_der(asn_val) + except ValueError as err: + raise SaltInvocationError( + f"Failed parsing OpenSSL otherName value {value!r}" + ) from err + return cx509.OtherName(oid, encoded) + + if not isinstance(value, dict): + raise SaltInvocationError( + f"Invalid otherName definition, dict or string required, got {value!r}" + ) + if "oid" not in value: + raise SaltInvocationError("Invalid otherName definition, missing `oid` key") + oid = _get_oid(value["oid"]) + + if "der" in value: + if isinstance(value["der"], bytes): + encoded = value["der"] + elif value["der"].startswith("hex:"): + try: + encoded = bytes.fromhex(value["der"].removeprefix("hex:")) + except ValueError as err: + raise SaltInvocationError( + "Failed to parse otherName `der` input as hex" + ) from err + elif value["der"].startswith("b64:"): + try: + encoded = base64.b64decode(value["der"].removeprefix("b64:")) + except ValueError as err: + raise SaltInvocationError( + "Failed to parse otherName `der` input as base64" + ) from err + else: + raise SaltInvocationError( + "Failed to parse otherName `der` input, needs `hex:` or `b64:` prefix" + ) + return cx509.OtherName(oid, encoded) + if "value" in value: + # Support basic types by passing them through + to_encode = value["value"] + if to_encode is None: + to_encode = asn1.Null() + try: + encoded = asn1.encode_der(to_encode) + except ValueError as err: + raise SaltInvocationError( + f"Failed to encode otherName value {value['value']!r} to ASN1" + ) from err + return cx509.OtherName(oid, encoded) + raise SaltInvocationError( + "Invalid otherName definition, missing `value` or `der` key" + ) + + def _parse_general_names(val): def idna_encode(val, allow_leading_dot=False, allow_wildcard=False): # A leading dot is allowed in some values (nameConstraints). @@ -1915,7 +2010,7 @@ def idna_encode(val, allow_leading_dot=False, allow_wildcard=False): "rid": cx509.general_name.RegisteredID, "ip": cx509.general_name.IPAddress, "dirname": cx509.general_name.DirectoryName, - # othername currently not implemented + "othername": _parse_other_name, } parsed = [] @@ -1953,8 +2048,6 @@ def idna_encode(val, allow_leading_dot=False, allow_wildcard=False): ) elif typ == "dns": v = idna_encode(v, allow_leading_dot=True, allow_wildcard=True) - elif typ == "othername": - raise SaltInvocationError("otherName is currently not implemented") if typ in valid_types: try: parsed.append(valid_types[typ](v)) @@ -2067,6 +2160,12 @@ def render_gn(gn): return f"RID:{gn.value.dotted_string}" if isinstance(gn, cx509.UniformResourceIdentifier): return f"URI:{gn.value}" + if isinstance(gn, cx509.OtherName): + try: + val = "UTF8:" + asn1.decode_der(str, gn.value) + except ValueError: + val = f"" + return f"otherName:{gn.type_id.dotted_string};{val}" return str(gn) diff --git a/tests/pytests/functional/modules/test_x509_v2.py b/tests/pytests/functional/modules/test_x509_v2.py index e51001b4b6ad..394b34987e02 100644 --- a/tests/pytests/functional/modules/test_x509_v2.py +++ b/tests/pytests/functional/modules/test_x509_v2.py @@ -850,7 +850,7 @@ def test_create_certificate_with_extensions(x509, ca_key, ca_cert, rsa_privkey): "authorityKeyIdentifier": "keyid:always", "issuerAltName": "DNS:salt.ca", "authorityInfoAccess": "OCSP;URI:http://ocsp.salt.ca/", - "subjectAltName": "DNS:sub.salt.ca,email:sub@salt.ca", + "subjectAltName": "DNS:sub.salt.ca,email:sub@salt.ca,otherName:1.2.3.4;UTF8:foobar", "crlDistributionPoints": "URI:http://salt.ca/myca.crl", "certificatePolicies": "1.2.4.5", "policyConstraints": "requireExplicitPolicy:3", diff --git a/tests/pytests/functional/states/test_x509_v2.py b/tests/pytests/functional/states/test_x509_v2.py index f420a0fd5c95..b0cb774ec377 100644 --- a/tests/pytests/functional/states/test_x509_v2.py +++ b/tests/pytests/functional/states/test_x509_v2.py @@ -505,7 +505,11 @@ def cert_args_exts(): "authorityKeyIdentifier": "keyid:always", "issuerAltName": "DNS:salt.ca", "authorityInfoAccess": "OCSP;URI:http://ocsp.salt.ca/", - "subjectAltName": "DNS:sub.salt.ca,email:sub@salt.ca", + "subjectAltName": [ + "DNS:sub.salt.ca", + {"email": "sub@salt.ca"}, + {"othername": {"oid": "1.2.3.4", "value": True}}, + ], "crlDistributionPoints": "URI:http://salt.ca/myca.crl", "certificatePolicies": "1.2.4.5", "policyConstraints": "requireExplicitPolicy:3", diff --git a/tests/pytests/unit/utils/test_x509.py b/tests/pytests/unit/utils/test_x509.py index 5b8113a08e1e..7023d3c1f963 100644 --- a/tests/pytests/unit/utils/test_x509.py +++ b/tests/pytests/unit/utils/test_x509.py @@ -1,3 +1,4 @@ +import base64 import ipaddress from datetime import datetime, timedelta, timezone @@ -11,6 +12,9 @@ "cryptography", reason="Needs cryptography library", minversion="37.0" ) cx509 = pytest.importorskip("cryptography.x509", reason="Needs cryptography library") +asn1 = pytest.importorskip( + "cryptography.hazmat.asn1", reason="Needs cryptography library" +) cprim = pytest.importorskip( "cryptography.hazmat.primitives", reason="Needs cryptography library" ) @@ -1272,9 +1276,101 @@ def test_create_invalidity_date(self, val, expected, critical): "Failed parsing rfc4514 dirName string", ), ( - ("otherName", "otherName:1.2.3.4;UTF8:some other identifier"), - salt.exceptions.SaltInvocationError, - "otherName is currently not implemented", + ("otherName", "1.2.3.4;UTF8:some other identifier"), + cx509.OtherName, + ( + cx509.ObjectIdentifier("1.2.3.4"), + asn1.encode_der("some other identifier"), + ), + ), + ( + ( + "otherName", + "1.3.6.1.5.5.7.8.9;FORMAT:UTF8,UTF8String:nonasciinäme.example.com", + ), + cx509.OtherName, + ( + cx509.ObjectIdentifier("1.3.6.1.5.5.7.8.9"), + asn1.encode_der("nonasciinäme.example.com"), + ), + ), + ( + ("otherName", "1.2.3.4;BOOL:TRUE"), + salt.exceptions.CommandExecutionError, + ".*only UTF8STRING is supported.*", + ), + ( + ("otherName", {"oid": "1.2.3.4", "value": "some other identifier"}), + cx509.OtherName, + ( + cx509.ObjectIdentifier("1.2.3.4"), + asn1.encode_der("some other identifier"), + ), + ), + ( + ("otherName", {"oid": "1.2.3.4", "value": True}), + cx509.OtherName, + (cx509.ObjectIdentifier("1.2.3.4"), asn1.encode_der(True)), + ), + ( + ("otherName", {"oid": "1.2.3.4", "value": None}), + cx509.OtherName, + (cx509.ObjectIdentifier("1.2.3.4"), asn1.encode_der(asn1.Null())), + ), + ( + ( + "otherName", + { + "oid": "1.2.3.4", + "der": "hex:" + asn1.encode_der("hex encoded utf8string").hex(), + }, + ), + cx509.OtherName, + ( + cx509.ObjectIdentifier("1.2.3.4"), + asn1.encode_der("hex encoded utf8string"), + ), + ), + ( + ( + "otherName", + { + "oid": "1.2.3.4", + "der": "b64:" + + base64.b64encode( + asn1.encode_der( + "base64 encoded utf8string, but arbitrary types are allowed" + ) + ).decode(), + }, + ), + cx509.OtherName, + ( + cx509.ObjectIdentifier("1.2.3.4"), + asn1.encode_der( + "base64 encoded utf8string, but arbitrary types are allowed" + ), + ), + ), + ( + ("otherName", []), + salt.exceptions.CommandExecutionError, + ".*dict or string required.*", + ), + ( + ("otherName", {}), + salt.exceptions.CommandExecutionError, + ".*missing `oid` key.*", + ), + ( + ("otherName", {"oid": "1.2.3.4"}), + salt.exceptions.CommandExecutionError, + ".*missing `value` or `der` key.*", + ), + ( + ("otherName", {"oid": "1.2.3.4", "der": "foobar"}), + salt.exceptions.CommandExecutionError, + ".*needs `hex:` or `b64:` prefix.*", ), ( ("invalidType", "L'état c'est moi!"), @@ -1288,7 +1384,10 @@ def test_parse_general_names(inpt, cls, parsed): with pytest.raises(cls, match=parsed): x509._parse_general_names([inpt]) return - expected = cls(parsed) + if inpt[0] == "otherName": + expected = cls(*parsed) + else: + expected = cls(parsed) res = x509._parse_general_names([inpt]) if inpt[0] == "dirName": assert res[0].value == expected @@ -1631,11 +1730,28 @@ def test_get_dn(inpt, expected): cx509.Extension( cx509.SubjectAlternativeName.oid, value=cx509.SubjectAlternativeName( - [cx509.DNSName("example.io"), cx509.RFC822Name("hello@example.io")] + [ + cx509.DNSName("example.io"), + cx509.RFC822Name("hello@example.io"), + cx509.OtherName( + cx509.ObjectIdentifier("1.2.3.4"), asn1.encode_der("foobar") + ), + cx509.OtherName( + cx509.ObjectIdentifier("1.2.3.4.5"), asn1.encode_der(True) + ), + ] ), critical=False, ), - {"critical": False, "value": ["DNS:example.io", "email:hello@example.io"]}, + { + "critical": False, + "value": [ + "DNS:example.io", + "email:hello@example.io", + "otherName:1.2.3.4;UTF8:foobar", + "otherName:1.2.3.4.5;", + ], + }, ), ( cx509.Extension(