[misiektoja]

Description

The documentation of "CA MUST Reject IR With Invalid CertReqId" now names badRequest, the bit the test checks. It also cites RFC 9483 Section 4.1.1 instead of 4.1.3.

Motivation and Context

The documentation said badDataFormat or badRequest, but the test has required badRequest since e2f86a4. The certReqId rule for an ir is in Section 4.1.1. Section 4.1.3 covers kur.

How Has This Been Tested?

robot --dryrun, robocop check and codespell pass for tests/lwcmp.robot. The test passes against the mock CA, which answers with badRequest. Python 3.15 with Robot Framework 7.5.

[ralienpp]

@misiektoja, you're right - the docstring says both are accepted, but only one is checked.

The RFC itself doesn't state explicitly what bit to set here (though maybe I'll change my mind if I read it N more times). Given the ambiguity, what makes you confident that it isn't the docstring that must be corrected?

[misiektoja]

The RFC itself doesn't state explicitly what bit to set here (though maybe I'll change my mind if I read it N more times). Given the ambiguity, what makes you confident that it isn't the docstring that must be corrected?

Good question! The ambiguity is why I changed the check rather than the docstring.

Section 4.1.1 gives the rule (certReqId MUST be 0), so the rejection is mandatory. Which failInfo bit to set is a separate question that RFC 9483 leaves open. Section 3.5 lists its bits as MAY and has none for certReqId. Section 3.6.4 defers to Appendix F of RFC 4210, whose one-line descriptions fit both badRequest and badDataFormat. Section 3.5 even uses badDataFormat for a profile rule that is not a syntax error (the missing transactionID), which is the same kind of violation.

So I'm not claiming badDataFormat is the right bit, only that the RFC does not make it wrong. A MUST test should not fail a CA on a choice the spec leaves open. OpenSSL’s CMP server and the mock CA send badRequest, so this change does not affect them.

If you'd still rather require badRequest only, I’ll change the PR to fix the docstring instead. Either way the docstring should cite 4.1.1 rather than 4.1.3, which covers kur.

[ralienpp]

@misiektoja what do you think should be the behaviour of PKIStatusInfo failinfo Bit Must Be when exclusive=True and multiple1` bits are given?

[misiektoja]

@misiektoja what do you think should be the behaviour of PKIStatusInfo failinfo Bit Must Be when exclusive=True and multiple1` bits are given?

Good question! I read it as “at least one listed bit and no bit outside the list”. That matches the keyword doc (“no other failinfo bits are set”) and fits a BIT STRING that may carry several reasons.

The implementation differs when more than one listed bit is set. If all listed bits are set, the result depends on the order of the names: badRequest,badDataFormat (support_messages.robot) passes, but badDataFormat,badRequest (this PR) fails. With three names, any two set fails in either order. A single listed bit passes in either order and an unlisted bit always fails. 13 calls in tests/ pass several names with exclusive=True, so they all depend on this.

I can swap the order here to match support_messages.robot. In a separate PR I can make is_bit_set follow the rule above, with unit tests. What do you think?

[ralienpp]

@misiektoja, I have some doubts.

The docstring says "bit(s) that must be set", there is no hint there that the logic is "if there are several bits AND exclusive is true, then at least one must be set" (rather than "both").

Your interpretation differs from mine, and the fact that there are several ways to see it suggests that there is room for improvement in terms of usability. Do you agree? How would you approach it?

The other thing is that the order of the bits shouldn't matter, if it fails with badDataFormat,badRequest but works with badRequest,badDataFormat - that's probably not a good thing. Don't you operate under the assumption that the order of fail bits should not influence the outcome?

[misiektoja]

Yes to both! The keyword doc reads as “all of these”, the order must not matter and swapping the order here was only a workaround. The real fix belongs in is_bit_set.

The tests that document several bits say “X or Y” (cert_conf_tests.robot, pki_mgmt_entity_op.robot), the is_bit_set docstring says “only one bit must be set” and the helper is _is_either_bit_set_in_bitstring. Only the keyword doc hints at “all of these”, so essentially the three disagree today 🙂

What do you thinking about fixing it in a following way (separate PR):

• Document the keyword as “at least one listed bit is set” and exclusive=True as “no bit outside the list is set”.
• Compare sets, so the order never matters.
• Unit tests: single bit, several bits in both orders, subsets, unlisted bits.
• No “all of these” mode, since no test needs it. It can be added later as match=all.

[ralienpp]

What do you thinking about fixing it in a following way (separate PR):

@misiektoja, thanks for the suggestion, I suggest a smaller incremental step first: please correct the existing keyword such that we fix the "order should not matter" problem, leave the docstring unchanged.

"At least one listed bit is set" should probably be a separate keyword, but I still haven't decided what the best UX would be. The suggestion to change the doc as you propose is probably not good - because what if I REALLY need "exactly one bit is set" behaviour? You'll end up in a similar problem.

p.s. by the way, who is your platoon commander?

[misiektoja]

@misiektoja, thanks for the suggestion, I suggest a smaller incremental step first: please correct the existing keyword such that we fix the "order should not matter" problem, leave the docstring unchanged.

"At least one listed bit is set" should probably be a separate keyword, but I still haven't decided what the best UX would be. The suggestion to change the doc as you propose is probably not good - because what if I REALLY need "exactly one bit is set" behaviour? You'll end up in a similar problem.

p.s. by the way, who is your platoon commander?

Thanks. #116 fixes only the order. is_bit_set now compares the names as sets and the docstring is unchanged. Four calls in tests/ list their bits out of bit order, for example transactionIdInUse,badRequest. With the fix they pass when the CA sets all listed bits, as badRequest,badDataFormat already did.

And I agree that "at least one listed bit" belongs in a separate keyword. A caller may really need exactly one.

I hope it is ok now.

P.S. No platoon, just one soldier with a backlog that finally got some time off :-)