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

minor updates to arguments and error reporting methods. #1

Closed
wants to merge 2 commits into from

Conversation

pyn
Copy link

@pyn pyn commented Feb 9, 2015

No description provided.

@jschauma
Copy link
Owner

jschauma commented Feb 9, 2015

The switch from '-c' to '-f' seems cosmetic, but would break backwards compatibility, so I'd be inclined to leave it at '-c', unless there's a good reason to change it. (If changed, it'd also require an update to the manual page.)

Since error() is only called from within a subshell, it won't actually exit; however, the intention of it exiting should be preserved, so that we'd probably want a

CERTS=$(verifyArg "${OPTARG}")
if [ $? -gt 0 ]; then
exit 1
fi

block, right?

@pyn
Copy link
Author

pyn commented Feb 9, 2015

With regard to '-c' vs. '-f' the problem is subtle but I believe there is a non-cosmetic issue, i.e.:

ssh -t scanuser@fqdn -- /dev/tty

Will yield the following command being executed:

openssl smime -verify -inform pem -CAfile /dev/tty

At this point ANY arbitrary script and cert can be provided on STDIN and the shell will happily execute.

As far as exiting goes, yes the intention of it exiting should be preserved. We can put that in an if block instead of using && exit 1 if that makes more sense.

@jschauma
Copy link
Owner

Ah, the reason is that a login shell will be invoked with "-c" at login time if args are present. Alright, that makes sense now. :-)

I'll merge this together with a fix to the manual page and adding the exiting on errors later today.

Thanks!

@jschauma
Copy link
Owner

I just committed the fixes you suggested together with another test case and the update to the man page.
Thanks again for submitting the request!

@jschauma jschauma closed this Feb 10, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants