Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apps: Stop pretending to care about Netscape keys #7440

Conversation

levitte
Copy link
Member

@levitte levitte commented Oct 18, 2018

The documentation says some commands care, but the code says differently.

The documentation says some commands care, but the code says differently.
@levitte levitte added branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch labels Oct 18, 2018
@richsalz
Copy link
Contributor

Nice!

@mspncp
Copy link
Contributor

mspncp commented Oct 18, 2018

LGTM. But I'm in doubt whether this change is allowed to be backported to 1.1.1: even if the option was previously ignored (effectively), after your change the command fails because an unknown option is passed. This could break existing scripts. It might be unlikely that this is the case, but formally, it is a breaking change.

Copy link
Contributor

@paulidale paulidale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For master only. I agree that for 1.1.1 it is an API change.

@paulidale paulidale removed the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label Oct 19, 2018
@levitte
Copy link
Member Author

levitte commented Oct 19, 2018

Ah... I could leave the option in with a comment, in apps/opt.c

@mspncp
Copy link
Contributor

mspncp commented Oct 19, 2018

IMO ignoring the option is ok if it didn't work previously anyway. Would it be reasonable to a warning on stderr, too?

@levitte
Copy link
Member Author

levitte commented Oct 19, 2018

It's been ignored since July 2015, as off 0bc2f36

So basically, the last time it was at all used was with 1.0.2

@levitte
Copy link
Member Author

levitte commented Oct 19, 2018

BTW, this doesn't break anything in 1.1.1, it's already broken in this respect (and [ahem] hilariously so, I might add):

: ; ./util/shlib_wrap.sh openssl rsa -in ../1.1.1/test/testrsa.pem -out foo.ns -outform netscape
rsa: Bad format "netscape"; must be one of:
   PEM/DER
   pkcs12
   smime
   engine
   msblob
   netscape
   nss
   text
   http
   pvk
rsa: Invalid format "netscape" for -outform
rsa: Use -help for summary.
: ; ./util/shlib_wrap.sh openssl rsa -in ../1.1.1/test/testrsa.pem -out foo.ns -outform net
rsa: Bad format "net"; must be one of:
   PEM/DER
   pkcs12
   smime
   engine
   msblob
   netscape
   nss
   text
   http
   pvk
rsa: Invalid format "net" for -outform
rsa: Use -help for summary.

So I still think this should be cherry-picked back to 1.1.1.

@levitte levitte added the branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch label Oct 19, 2018
@mspncp
Copy link
Contributor

mspncp commented Oct 19, 2018

Replacing a failure by a more sensible failure is not a regression, it's a bugfix. So in this case, go ahead to cherry-pick it, if @paulidale approves.

@levitte
Copy link
Member Author

levitte commented Oct 21, 2018

@paulidale, after the deliberation here, do you approve cherry-picking back to 1.1.1?

@paulidale
Copy link
Contributor

Yes, 1.1.1 is good too. Broken is broken and I don't mind why.

@paulidale paulidale added the approval: done This pull request has the required number of approvals label Oct 21, 2018
levitte added a commit that referenced this pull request Nov 2, 2018
The documentation says some commands care, but the code says differently.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #7440)

(cherry picked from commit d91d443)
@levitte
Copy link
Member Author

levitte commented Nov 2, 2018

Merged.

master:
d91d443 apps: Stop pretending to care about Netscape keys

1.1.1:
b33e769 apps: Stop pretending to care about Netscape keys

@levitte levitte closed this Nov 2, 2018
levitte added a commit that referenced this pull request Nov 2, 2018
The documentation says some commands care, but the code says differently.

Reviewed-by: Paul Dale <paul.dale@oracle.com>
(Merged from #7440)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch branch: 1.1.1 Merge to OpenSSL_1_1_1-stable branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants