Hi Quentin, Em qui., 18 de jun. de 2026 às 14:42, Quentin Schulz < [email protected]> escreveu: > > I think this could be split in multiple commits. I could identify the > following unrelated changes: [...] > Do you agree with this split?
Yes, that's a fair split, will do in v6. And you're right on 1): the plain PKCS#11 URI case already works today without any code change, since binman passes keyfile through to 'openssl -key' unchanged and the provider configured via OPENSSL_CONF resolves it (with the PIN coming from openssl.cnf or a pin-value attribute in the URI). So the first patch of the split will be docs plus a test exercising exactly that. > Should we rather move this logic to ReadNode() instead then? Hopefully > that is run only once? What made you chose GetCertificate() for this > logic? Maybe I'm missing some clue. I wanted to avoid mutating entry state given the double GetCertificate() call, hence the local variable. But ReadNode() runs once, so moving the logic there is simpler; will do in v6. I'll also make the helper skip appending when the URI already carries a pin-value attribute, so it becomes idempotent either way. > This isn't specific to HSMI/PKCS11 though, is it? So it's just a way to > override the keyfile entry argument by an environment variable. Yes, the > typical usecase is to use it for an HSM but nothing forces you to use it > for an HSM? Correct, it's a generic override. I'll reword the documentation and rename the variable to BINMAN_X509_KEYFILE in v6 so the name matches the entry argument it overrides (it can carry a plain key file path just as well as a URI). Shout if you prefer the old name or a different one. > Aren't we missing "local PEM key file" here? Indeed, will restructure the list as the accepted forms of 'keyfile': local key file (default), PKCS#11 URI via provider, engine-prefixed URI. > Is it requiring OpenSSL 3.x though or did you only test on OpenSSL 3.x? > Since engines were supported in OpenSSL 1.x, I would assume PKCS11 > signing also worked back then? Are you aware of a limitation? Also, does > it NOT work with OpenSSL 4.x (which distro will slowly start to migrate to)? It is a hard requirement, and it follows from the design we settled on in the v3 review: binman never passes -engine/-keyform itself (the v3 auto-detection machinery that could have done so was dropped on your suggestion), so the key must be resolvable through a bare 'openssl -key <uri>'. That only works with the provider API or the org.openssl.engine: STORE scheme, both introduced in OpenSSL 3.0. On 1.x there is no transparent path, hence "OpenSSL 1.x is not supported." As for OpenSSL 4.x: the ENGINE API was removed entirely in OpenSSL 4.0 [1], so the engine-prefixed form is OpenSSL 3.x only; the provider path is the way forward and expected to work unchanged on 4.x. I'll reword the docs to state all of this explicitly. [1] https://openssl-library.org/post/2025-12-18-remove-engines/ > Yeah, or just say "OpenSSL 1.x is not supported." and be done with it :) Will do :) > question: why suddenly the variable before `make`? All the others are > after so it seems like an odd choice to me. It was deliberate: PKCS11_PIN is an environment variable read by binman itself (via os.environ), not a make variable interpreted by the build system. In practice `make PKCS11_PIN=1234` also works since make exports command-line variables to recipes. I'll add a sentence making that explicit. > The third option is to have the pin in the URI directly (which is > equally bad, but is possible no?). Yes, that works today (it's the "no code change" case above); I'll document it. > I'm wondering if we should support replacing the PIN inside the URI if > there's one in it and we also have PKCS11_PIN. I'd rather define PKCS11_PIN strictly as a fallback: if the URI already contains a pin-value attribute, the URI wins and PKCS11_PIN is ignored (right now the code would blindly append a second pin-value, which RFC 7512 doesn't allow). Will implement, document and unit-test that precedence in v6. Silently replacing would be surprising, IMO. > Side question, what happens if we have a pin set in openssl.cnf and also > via PKCS11_PIN/pin in the URI? I'll test this with softhsm2 and document the observed precedence in v6. I expect the URI pin-value to take precedence over pkcs11-module-token-pin, but I want to confirm before writing it down. > Idly wondering if we shouldn't do a simple try:except and let > raise_on_error to True and skip in the except. I have a mild preference for the explicit return-code check, since a missing provider is an expected outcome rather than an error. But I don't feel strongly, happy to change it if you do. > Please add a test for when the pin is passed via the URI directly (as I > assume this is supported) and not through the env variable. Will do. > We should add a keyfile property in the test device tree to make sure we > override it with the entryarg. Will do. I confirmed entry arguments take precedence over the device tree property (Entry.GetArg() checks entry args first). > It'd be nice to check the engine works as well with the prefix. [...] Simon suggested the same compromise; I'll add a unit test that the PIN combiner leaves the org.openssl.engine:pkcs11: prefix intact, plus a check that the prefixed URI reaches the openssl invocation unchanged. I am going on vacation and will be back in a few weeks, so I'll address all of the above and send v6 once I return. In the meantime, if you have any additional comments before I prepare v6, please let me know. Thanks a lot for the thorough review! Sergio Prado
