Reject duplicate -addext parameters #6636
Conversation
richsalz
added
branch: master
| my $val = "subjectAltName=DNS:example.com"; | ||
| my $val2 = " " . $val; | ||
| my $val3 = $val; | ||
| $val3 =~ s/=/ =/; |
There was a problem hiding this comment.
How about space after the = sign?
There was a problem hiding this comment.
Not needed, because only duplicate keys are checked. So only look for leading/trailing whitespace in the key of 'key=value' Make sense?
apps/req.c
Outdated
| * right. Return 0 if unique, -1 on runtime error; 1 if found or a syntax | ||
| * error. | ||
| */ | ||
| static int duplicated(STACK_OF(OPENSSL_STRING) *list, char *kv) |
There was a problem hiding this comment.
How about using a LHASH instead of a stack?
The strings are only ever checked for existence and a hash is ideal for that.
Plus, a hash of strings is already defined in libcrypto.
I doubt the performance difference will make a big difference here but avoiding a future gotcha is often good.
There was a problem hiding this comment.
Right you are. Additional commit pushed (to be squashed).
|
Thanks for the suggestions; merged. |
Reviewed-by: Paul Dale <paul.dale@oracle.com> (Merged from #6636)
| ok(!run(app(["openssl", "req", "-new", "-addext", $val, "-addext", $val]))); | ||
| ok(!run(app(["openssl", "req", "-new", "-addext", $val, "-addext", $val2]))); | ||
| ok(!run(app(["openssl", "req", "-new", "-addext", $val, "-addext", $val3]))); | ||
| ok(!run(app(["openssl", "req", "-new", "-addext", $val2, "-addext", $val3]))); |
There was a problem hiding this comment.
-addext doesn't work at all now. I mean not even if you pass one. This is because lh_TYPE_insert returns NULL on success [and error].
On related note it should be noted that these tests don't actually prove anything. Indeed, imagine that req erroneously pass duplicates, what would happen? If executed in terminal test will hang asking for PEM pass phrase, i.e. ultimately fail.
There was a problem hiding this comment.
They are negative tests, they show that multiple copies fail. There were no positive tests, so when I broke the working case the tests did not show it. Thanks for fixing it. These tests are still useful, as that is the original "feature" I was adding.
There was a problem hiding this comment.
Purpose of the tests is to catch things, right? In order to catch you need distinguishable outcomes. In this case application's exit code is used. Point is that it will always return same condition, error, and tests is bound to succeed no matter what. I mean it returns error with single -addext and with duplicate -addext options, and you can't tell failure and success apart. They should be formed in a way that if duplicate -addext is omitted, test would actually succeed.
There was a problem hiding this comment.
Yes, there should be a test to make sure that single addext works. I didn't do that. And I broke the code, apparently.
Fixes #5037