Revert -out option checks #6033

levitte · 2018-04-20T10:36:41Z

The checks were over elaborate and became so increasingly the more we kept trying.

This reverses #4008 and #3709, i.e. reinstates that very light checks, but keeps test/recipes/15-test_out_option.t. This puts us back at where we started with #3404 and allows us to deal with it differently that we have 'til now. This also fixes #5590, and cancels out #5592 and #5638.

The first commit is from #6031

Test writing to the null device. This should be successful. Test writing to a writable file in a write protected directory. This should be successful. Also, refactor so the planned number of tests is calculated. [extended tests]

levitte · 2018-04-20T10:40:17Z

@petrovr, I know this has been a pet peeve. Your comments would be appreciated.

richsalz

I think it's time to just walk away from this whole issue for this release, so this PR is good.

bernd-edlinger · 2018-04-22T11:15:16Z

There appveyor failure seems relevant:

[00:12:33] Generating RSA private key, 16384 bit long modulus (2 primes)
[00:12:33] ...................................................................................................................++
[00:12:33] ....................................................................................................................++
[00:12:33] e is 65537 (0x010001)
[00:12:33] ..\..\apps\openssl.exe genrsa -out test_out_option-nowrite-2628\unwritable.pem 16384 => 0
[00:12:33] not ok 9 - invalid output path: test_out_option-nowrite-2628\unwritable.pem
[00:12:33] 
[00:12:33] #   Failed test 'invalid output path: test_out_option-nowrite-2628\unwritable.pem'
[00:12:33] #   at ..\test\recipes\15-test_out_option.t line 82.
[00:12:33] not ok 10 - check time consumed
[00:12:33] 
[00:12:33] #   Failed test 'check time consumed'
[00:12:33] #   at ..\test\recipes\15-test_out_option.t line 85.

bernd-edlinger · 2018-04-22T11:19:13Z

also the travis mingw test is failing in a similar way:

genrsa: Cannot open output file /dev/null, Invalid argument

genrsa: Use -help for summary.

wine ../../apps/openssl.exe genrsa -out /dev/null 2048 => 1
not ok 14 - valid output path: /dev/null

levitte · 2018-04-22T17:32:54Z

@bernd-edlinger, I've added a fixup that should help.

levitte · 2018-04-23T04:06:45Z

Hmmm, is it just me, or does MSVC's _access not recognise NUL? @dot-asm, you seem to know these things better than most, would you be willing to shed some light on this?

dot-asm · 2018-04-23T21:51:28Z

is it just me, or does MSVC's _access not recognise NUL?

Hmmm... It apparently varies with CRT version... I don't know that it changed, but latest CRT apparently returns EINVAL upon attempt to _access("NUL"). On the other hand default CRT, c:\windows\system32\msvcrt.dll, one mingw links with, acknowledges NUL's existence and recognizes it as writable... I suppose MSC's _access is not to be trusted...

levitte · 2018-04-24T07:16:39Z

So basically, we (and any application, really) have to handle the magic file "nul" ourselves. That's kinda crappy... But ok, I can add that. Do we have to be equally attentive with the rest of the "POSIX API" (thinking of _open in particular)?

bernd-edlinger · 2018-04-24T07:23:19Z

Why do we have to ask first if we can open NUL?

levitte · 2018-04-24T07:29:08Z

Because the check of -out parameter calls app_access on its argument. I think we could remove that check, really, but haven't dared to, yet. Either way, we may have to handle "nul" specifically when opening the file...

bernd-edlinger · 2018-04-24T07:30:59Z

and "con", "com1", etc...?
and "\\.\nul" as well?

levitte · 2018-04-24T08:02:10Z

Good question, and I would rather not have to, so alternate solutions are welcome.
/notawindowsguy

bernd-edlinger · 2018-04-24T10:04:59Z

Me neither....
I haven't looked at the code yet, but my feeling is we should probably remove the _access test,
maybe instead only check that the string is not empty, and be prepared that open can fail.

dot-asm · 2018-04-24T10:23:16Z

Ready for another surprise? \\.\GLOBALROOT\device\null. No, double-l is not a typo.

If I may. First step is done, user is claimed to be ultimately responsible for actions. Just make the next step, omit the test. It's user responsibility even to pick the suitable file name for the environment, it's really between user and underlying OS. To be completely honest, I'd even go as far as arguing for omitting whole recipe. What is it that is tested really? OS ability to tell apart files from directories? Ability to note non-existing directory? Are these actually our problems? Ability to chmod 0555 to make directory non-writable? Well, even on Linux it's actually file-system-specific. For example, you can chmod 0555 a directory on vfat as much as you like, it won't make any difference... You can argue that nobody would run test on vfat file system, but then nobody would use /dev/null as output for key generation either....

dot-asm · 2018-04-24T10:30:50Z

For example, you can chmod 0555 a directory on [Linux] vfat as much as you like, it won't make any difference...

Same can be said about Windows, either FAT or NTFS. chmod 0555 adds read-only attribute to directory, but it has no effect on your ability to create files in the directory.

levitte · 2018-04-24T11:58:25Z

I wouldn't remove the test yet, I wanna see that our code can handle what the user throws at us correctly (i.e in a manner that a user should be able to expect, such as "nul" on the command prompt being correctly interpreted as the null device and not generating a silly error).
Considering _access doesn't seem to handle "nul" (i.e treats it as invalid), I'm questioning the whole "POSIX API" (including _open), and until we have shown for ourselves that we are treating this right, I think the recipe should stay. Perhaps not in its entirety, but still...

dot-asm · 2018-04-24T12:34:13Z

I wouldn't remove the test yet,

Then for negative testing I'd say one directory is sufficient, ".", and test for file in non-existing directory. I'd also say that there is no need to test for time (it's unreliable anyway), just check for exit code would do.

I wanna see that our code can handle what the user throws at us correctly (i.e in a manner that a user should be able to expect, such as "nul" on the command prompt being correctly interpreted as the null device and not generating a silly error).

Do [Windows] users actually have such specific expectations? Besides, you really can't do better than underlying system and systems do perform differently. So that there is no such thing as universal expectations, they have to be adjusted to specifics.

I'm questioning the whole "POSIX API" (including _open)

_open can open "nul". Because it actually translates to native system call. But what "POSIX API"? Does POSIX specify "_access" or "_open"? No. It's "access" and "open". What Microsoft provides is a porting aid, not actual POSIX API. It has limitations, and user should be aware of them. Once again, you can't do better than underlying system.

levitte · 2018-04-24T12:44:50Z

But what "POSIX API"? Does POSIX specify "_access" or "_open"? No. It's "access" and "open".

Exactly, that's why I wrote POSIX API within quotes, because it isn't really.

Do [Windows] users actually have such specific expectations?

I don't understand the question... in Unix and on VMS, there is a null device, and users on those platforms expect whatever they send that way to disappear. Are you telling me Windows users don't want to do something like that as well?

Reduce the amount of testing. We only need to check that one attempt to write to a directory fails, and we can skip checking writability to "protected" directories. Finally, we skip trying to check by timing, and we can further reduce the testing code to make it look less complex.

dot-asm · 2018-04-24T13:14:12Z

test/recipes/15-test_out_option.t

+
+foreach (@failure_paths) {
+    my $path = File::Spec->canonpath($_);
+    ok(!run(app([ 'openssl', 'genrsa', '-out', $path, '2048'])),


Does it have be such expensive operation? Of course it's hardly noticeable on your desktop, but do you have to make Raspberry Pi user suffer [more than necessary]? rand -out $path would do... Or why not convert something from pem to der?

Well, I suppose it applies more to success tests. That is if we assume that failure test would fail early....

The reason for it is that this was already in the recipe before I started fiddling with it, and I simply didn't think further. Of course we can change that to something better suited for quick checks...

use "openssl rand" rather than "openssl genrsa"

dot-asm · 2018-04-24T13:33:26Z

test/recipes/15-test_out_option.t

+# For example, if cross compiled and tested on the build host, perl will
+# generate an incorrect NULL device name.
+# We might expand the exceptions...
+unless (config('target') =~ m|^mingw| && $^O ne 'msys') {


Even though it's possible to test Linux cross-compiled targets with qemu-user (since they use same file system), it's not really inappropriate to simply assume that perl's devnull has no meaning to any cross-compiled target. Incidentally this will work out even for mingw builds performed under Linux and Cygwin. Because they are customarily performed with --cross-compile-prefix. In other words one can wonder if simple check for cross-compile prefix would be adequate. Do note "adequate", as this is really what it's about. I mean it's effectively impossible to specify condition that is guaranteed to work in all circumstances, you can only go for what has best chances to work in most cases. Of course it has to work with CIs, but as long as _access is out of picture check for cross-compile prefix should do the trick...

Oh, so now I should check config('CROSS_COMPILE')? 'cause back over here, you told me a different story.

... buuuuuuut ok, you say check cross compile prefix, I will check cross compile prefix.

Oh, so now I should check config('CROSS_COMPILE')?

I said "one can wonder if it would be adequate," which is not same [as to] say "do this" :-)

'cause back over here, you told me a different story.

But then I also said "Grrrr! MSYS2 cheats!" I.e. by that time I didn't actually realize that msys perl would convert "/dev/null" to "nul" when starting native binary such as "mingw". I thought that it would do same as Linux and Cygwin perls, i.e. actually pass "/dev/null".

"Grrrr! MSYS2 cheats!"

Besides, context is different now. That's what makes it hard to nail, which can be frustrating and appear contradicting at different occasions. It's multi-variable problem with no exact solution. Change one variable, like _access call removal, and suddenly check for cross-compile prefix makes more sense than other possible options. But it's just that it makes more sense, not that it's absolutely "the right thing to do ever"...

I said "one can wonder if it would be adequate," which is not same [as to] say "do this" :-)

True that... well, it does cover the mingw on Linux cases, and I've been wondering more than once what happens with other explicitly cross compiled environments. I think this covers quite a lot of the cases we commonly test for, though... right?

This reverts commit f6d7659.

This reverts commit 215a673.

This reverts commit 555c94a.

[extended tests]

open() will take care of the checks anyway

dot-asm · 2018-04-24T20:36:03Z

As soon as WIP is removed...

mspncp · 2018-04-24T21:18:27Z

i.e. lower their expectations.

For something as well know as "nul"? I think that's harsh, really.

I don't think it's necessary to lower their expectations. Peoply which are stuck with cmd.exe generally have very low expectations about their shell. ;-)

levitte · 2018-04-25T04:35:38Z

WIP removed.

dot-asm · 2018-04-25T08:47:55Z

test/recipes/15-test_out_option.t

+    './',
+   );
+my @success_paths = (
+    'test.pem'


Now with rand -out this is no longer suitable name. test.bin perhaps? Or random.bin? Or randomly generated name with .bin extension?

Ah, thanks for the reminder, that was on my mind before going to sleep yesterday

dot-asm

Red cross from appveyor is unrelated.

Test writing to the null device. This should be successful. Also, refactor so the planned number of tests is calculated. Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

This reverts commit f6d7659. Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

This reverts commit 215a673. Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

This reverts commit 555c94a. Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

[extended tests] Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

open() will take care of the checks anyway Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

levitte · 2018-04-25T09:46:13Z

Merged (including commit from #6031)

4522e13 apps/opt.c: Remove the access checks of input and output files
c36e909 Better check of return values from app_isdir and app_access
96de2e5 Revert "Check directory is able to create files for various -out option"
b47b665 Revert "Add VMS version of app_dirname()"
b9a354d Revert "Check on VMS as well"
39e32be test/recipes/15-test_out_option.t: refine tests

levitte added 2 commits April 20, 2018 11:37

test/recipes/15-test_out_option.t: add further tests

a5c91fd

Test writing to the null device. This should be successful. Test writing to a writable file in a write protected directory. This should be successful. Also, refactor so the planned number of tests is calculated. [extended tests]

fixup! test/recipes/15-test_out_option.t: add further tests

b4366c6

levitte force-pushed the revert-out-option-checks branch from dbc0ad6 to 90cca54 Compare April 20, 2018 10:38

levitte mentioned this pull request Apr 20, 2018

Defer creating output files in several apps #5638

Closed

2 tasks

richsalz approved these changes Apr 20, 2018

View reviewed changes

richsalz added the approval: done This pull request has the required number of approvals label Apr 20, 2018

fixup! test/recipes/15-test_out_option.t: add further tests

f8d1032

levitte force-pushed the revert-out-option-checks branch from 90cca54 to a9826e2 Compare April 22, 2018 17:32

fixup! test/recipes/15-test_out_option.t: add further tests

56ea9a1

levitte force-pushed the revert-out-option-checks branch from a9826e2 to e0aedb2 Compare April 23, 2018 02:16

levitte changed the title ~~Revert -out option checks~~ WIP: Revert -out option checks Apr 23, 2018

levitte force-pushed the revert-out-option-checks branch from b9a4482 to d6821b1 Compare April 24, 2018 12:57

dot-asm reviewed Apr 24, 2018

View reviewed changes

fixup! test/recipes/15-test_out_option.t: add further tests

fd68018

use "openssl rand" rather than "openssl genrsa"

levitte force-pushed the revert-out-option-checks branch from d6821b1 to cd7dcc6 Compare April 24, 2018 13:28

dot-asm reviewed Apr 24, 2018

View reviewed changes

levitte added 6 commits April 24, 2018 15:49

fixup! test/recipes/15-test_out_option.t: add further tests

2792bc3

Revert "Check on VMS as well"

48bc1cd

This reverts commit f6d7659.

Revert "Add VMS version of app_dirname()"

7b6e685

This reverts commit 215a673.

Revert "Check directory is able to create files for various -out option"

3586ac6

This reverts commit 555c94a.

Better check of return values from app_isdir and app_access

2e5247f

[extended tests]

apps/opt.c: Remove the access checks of input and output files

63be6d1

open() will take care of the checks anyway

levitte force-pushed the revert-out-option-checks branch from cd7dcc6 to 63be6d1 Compare April 24, 2018 13:54

levitte changed the title ~~WIP: Revert -out option checks~~ Revert -out option checks Apr 25, 2018

dot-asm reviewed Apr 25, 2018

View reviewed changes

dot-asm mentioned this pull request Apr 25, 2018

test/recipes/15-test_out_option.t: add further tests #6031

Closed

fixup! test/recipes/15-test_out_option.t: add further tests

74d6c7b

dot-asm approved these changes Apr 25, 2018

View reviewed changes

levitte added a commit that referenced this pull request Apr 25, 2018

Revert "Check on VMS as well"

b9a354d

This reverts commit f6d7659. Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

levitte added a commit that referenced this pull request Apr 25, 2018

Revert "Add VMS version of app_dirname()"

b47b665

This reverts commit 215a673. Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

levitte added a commit that referenced this pull request Apr 25, 2018

Revert "Check directory is able to create files for various -out option"

96de2e5

This reverts commit 555c94a. Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

levitte added a commit that referenced this pull request Apr 25, 2018

Better check of return values from app_isdir and app_access

c36e909

[extended tests] Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

levitte added a commit that referenced this pull request Apr 25, 2018

apps/opt.c: Remove the access checks of input and output files

4522e13

open() will take care of the checks anyway Reviewed-by: Andy Polyakov <appro@openssl.org> (Merged from #6033)

levitte closed this Apr 25, 2018

levitte mentioned this pull request Apr 25, 2018

apps/opt.c: refactor the check of the -out argument #5592

Closed

mattcaswell mentioned this pull request Apr 27, 2018

crl -out /dev/null gives permission denied #5764

Closed

paulidale mentioned this pull request Jun 21, 2018

Clobbers certificate file if certificate already exists #6544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revert -out option checks #6033

Revert -out option checks #6033

levitte commented Apr 20, 2018 •

edited

levitte commented Apr 20, 2018

richsalz left a comment

bernd-edlinger commented Apr 22, 2018

bernd-edlinger commented Apr 22, 2018

levitte commented Apr 22, 2018

levitte commented Apr 23, 2018

dot-asm commented Apr 23, 2018

levitte commented Apr 24, 2018

bernd-edlinger commented Apr 24, 2018

levitte commented Apr 24, 2018

bernd-edlinger commented Apr 24, 2018 •

edited

levitte commented Apr 24, 2018

bernd-edlinger commented Apr 24, 2018

dot-asm commented Apr 24, 2018

dot-asm commented Apr 24, 2018

levitte commented Apr 24, 2018

dot-asm commented Apr 24, 2018

levitte commented Apr 24, 2018

dot-asm Apr 24, 2018

dot-asm Apr 24, 2018

levitte Apr 24, 2018

dot-asm Apr 24, 2018

levitte Apr 24, 2018

dot-asm Apr 24, 2018 •

edited

dot-asm Apr 24, 2018

levitte Apr 24, 2018

dot-asm commented Apr 24, 2018

mspncp commented Apr 24, 2018

levitte commented Apr 25, 2018

dot-asm Apr 25, 2018

levitte Apr 25, 2018

dot-asm left a comment

levitte commented Apr 25, 2018

Revert -out option checks #6033

Revert -out option checks #6033

Conversation

levitte commented Apr 20, 2018 • edited

levitte commented Apr 20, 2018

richsalz left a comment

Choose a reason for hiding this comment

bernd-edlinger commented Apr 22, 2018

bernd-edlinger commented Apr 22, 2018

levitte commented Apr 22, 2018

levitte commented Apr 23, 2018

dot-asm commented Apr 23, 2018

levitte commented Apr 24, 2018

bernd-edlinger commented Apr 24, 2018

levitte commented Apr 24, 2018

bernd-edlinger commented Apr 24, 2018 • edited

levitte commented Apr 24, 2018

bernd-edlinger commented Apr 24, 2018

dot-asm commented Apr 24, 2018

dot-asm commented Apr 24, 2018

levitte commented Apr 24, 2018

dot-asm commented Apr 24, 2018

levitte commented Apr 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm Apr 24, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm commented Apr 24, 2018

mspncp commented Apr 24, 2018

levitte commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dot-asm left a comment

Choose a reason for hiding this comment

levitte commented Apr 25, 2018

levitte commented Apr 20, 2018 •

edited

bernd-edlinger commented Apr 24, 2018 •

edited

dot-asm Apr 24, 2018 •

edited