Migrating to x509_v2: Passed invalid arguments to x509.sign_remote_certificate

[OrangeDog]

I'm on 3006.9, trying to migrate my x509 states.
I have added features: {x509_v2: true} to the config of both the minion and the master and restarted.
However, I currently get the following error:

[ERROR] Received invalid return value from ca_server: Passed invalid arguments to x509.sign_remote_certificate: sign_remote_certificate() takes 1 positional argument but 3 were given

{{ pki.cert_path }}:
  x509.certificate_managed:
    - ca_server: salt
    - signing_policy: my-ca
    - public_key: {{ pki.key_path }}
    - days_valid: 30
    - days_remaining: 7
    - CN: {{ grains['fqdn'] }}
    - subjectAltName: "DNS:{{ grains['fqdn'] }}, DNS:{{ grains['host'] }}, {{ ip_names }}"
    - mode: '0644'

Am I missing a step somewhere or does it just not work in this version?

[lkubb]

It definitely works on 3006.

For some reason, the minion acting as the CA server does not use x509_v2, but the legacy module. See the positional arg counts:

salt/salt/modules/x509.py

Lines 1069 to 1072 in 9233e1c

	def sign_remote_certificate(argdic, **kwargs):
	"""
	Request a certificate to be remotely signed according to a signing policy.

salt/salt/modules/x509_v2.py

Lines 1859 to 1862 in 9233e1c

	def sign_remote_certificate(
	signing_policy, kwargs, get_signing_policy_only=False, **more_kwargs
	):
	"""

Did you enable it on the salt minion as well and restarted it?

[lkubb]

1. Sadly the error message is quite generic, but seems to originate from the minion running the state, not the remote one (it would have ca_server reported errors: as a prefix otherwise). You could try loading the passed public_key via x509.get_public_key and see if that works (or enable debug logging and investigate the call stack to know from where exactly salt.utils.x509.load_pubkey was called when this exception is thrown).

Edit: Noticed you linked an existing issue and posted there, I'll have a look.

2. The latter one seems to be a self-signed certificate, for which it's currently not possible to set issuer information (afair it's just a missing implementation, nothing fundamental):

salt/salt/utils/x509.py

Lines 1224 to 1237 in 9233e1c

	if val.get("issuer") == "always" or (
	"issuer" in val and args["key_identifier"] is None
	):
	try:
	args["authority_cert_issuer"] = [cx509.DirectoryName(ca_crt.issuer)]
	args["authority_cert_serial_number"] = ca_crt.serial_number
	except (AttributeError, ValueError) as err:
	# this will not work for self-signed certificates currently
	args["authority_cert_issuer"] = args["authority_cert_serial_number"] = None
	if val["issuer"] == "always":
	raise CommandExecutionError(
	"Could not add authority_cert_issuer and "
	"authority_cert_serial_number, but was set to always."
	) from err

[OrangeDog]

1. I enabled trace logging, and there was no mention of the error anywhere. I posted it to the linked issue.
2. The previous/current implementation supports it. Is that breaking change documented somewhere?

[lkubb]

1. Sorry, missed the link the first time. I suspect I found the cause, see the linked issue. If I'm correct, I don't how why this was missed and think it should be addressed.

2. It seems not, at least not specifically. Some context:

The previous modules put the burden of parsing the configuration on M2crypto/OpenSSL, which meant automatic support for everything OpenSSL supports, but also made it (nearly?) impossible hard/unreliable to check for changes without issuing a new certificate. To be backwards-compatible, the x509_v2 modules have to parse OpenSSL config strings and translate them into an intermediate representation, then into cryptography objects (you can pass proper Python data structures now, which I'd recommend when writing new states). A few extensions are not supported by cryptography at all and the parsing itself is hard to validate, which is why there is this message:

salt/salt/modules/x509_v2.py

Lines 132 to 133 in 9233e1c

	* The formatting of some extension definitions might have changed, but should
	be stable for most basic use cases.

However, it seems I was aware of this specific breakage when writing the code, but did not include it in the breaking changes. Either I forgot (rewriting x509 for cryptography was an immense amount of work I did by myself, it's likely there were some oversights, see 1) or decided it was too insignificant and would be reported promptly if it was not.

Do you consider this an important feature?

[OrangeDog]

Do you consider this an important feature?

I'm not really sure. It's "correct" for a self-signed certificate to have explicit metadata that it signed itself, but I don't think it really has much effect?

The more annoying consequence is that it will prematurely reissue my internal CA (and then everything that had signed) because it cannot match what it had created before. But that appears to apply to all the extensions and the ordering of the DNs:

          ID: /etc/ssl/CA/certs/internal.crt
    Function: x509.certificate_managed
      Result: None
     Comment: The certificate would have been recreated
     Started: 16:36:19.812149
    Duration: 1592.956 ms
     Changes:
              ----------
              extensions:
                  ----------
                  added:
                  changed:
                      - basicConstraints
                      - keyUsage
                      - subjectKeyIdentifier
                      - authorityKeyIdentifier
                  removed:
              issuer_name:
                  1.2.840.113549.1.9.1=...,CN=...,OU=...,O=...,L=...,ST=...,C=...
              subject_name:
                  1.2.840.113549.1.9.1=...,CN=...,OU=...,O=...,L=...,ST=...,C=...

The DN was previously ordered alphabetically: C, CN, emailAddress, L, O, OU, ST
The difference in basicConstraints and keyUsage is that critical has been added (which appears to have been a bug in the old version).
Keyids are expected to change because I also have it generating new keys when certs are changed.

So I guess it's always going to regenerate regardless.

you can pass proper Python data structures now

Like this?

 - basicConstraints:
   - critical
   - CA: false
 - subjectAltName: 
   - DNS: {{ grains['fqdn'] }}
   - DNS: {{ grains['host'] }}
{% for ip in salt['network.ip_addrs'](type='public') %}
   - IP: {{ ip }}
{% endfor %}

[lkubb]

Ah, I see. Yes, you won't get around reissuing the certificate in that case anyways, the new DN order is hardcoded. You could get around the subject DN causing a reissuance by specifying the old order explicitly in subject instead of relying on the CN... kwargs.

The spec even allows omitting the authorityKeyIdentifier completely in case of self-signed certificates, so it's compliant, but suboptimal (it's still adding the extension with the keyIdentifier field).

Mostly, yeah. Like this:

- basicConstraints:
    critical: true
    ca: false
- subjectAltName:
  - dns: {{ grains['fqdn'] }}  # uppercase works also
  - dns: {{ grains['host'] }}
{%- for ip in salt['network.ip_addrs'](type='public') %}
  - ip: {{ ip }}
{%- endfor %}

It's documented in the execution module somewhat: https://docs.saltproject.io/en/latest/ref/modules/all/salt.modules.x509_v2.html#salt.modules.x509_v2.create_certificate#:~:text=X.509v3%20extensions%20in%20kwargs