Page MenuHomeFreeBSD

Introduce certctl(8)
Needs ReviewPublic

Authored by kevans on Aug 23 2018, 3:46 AM.

Details

Summary

A simple utility to hash all trusted certificates on the system into /etc/ssl/certs
Also allows the user to blacklist certificates they do not trust

Todo: create man page

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 22595

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
dteske added inline comments.Aug 23 2018, 8:57 PM
usr.sbin/trustctl/trustctl.sh
126 ↗(On Diff #47148)

Add double-quotes around $@

128 ↗(On Diff #47148)

Add double-quotes around $BFILE

133–134 ↗(On Diff #47148)

Combine shift and comment to a single-line and only say what is being shifted:

shift # verb

We know shift removes arguments, just tell us what it was.

135 ↗(On Diff #47148)

Add double-quotes around $@

136 ↗(On Diff #47148)

Add double-quotes around $BFILE

137 ↗(On Diff #47148)

Add a single space after $( and before ).

138 ↗(On Diff #47148)

Remove unnecessary curlies around ${hash}

139 ↗(On Diff #47148)

Change to:

rm -f "$BLACKLISTDESTDIR/$hash.0"

Note that I would:
Add double-quotes around $BLACKLISTDESTDIR/${hash}.0.
Remove unnecessary curlies around ${hash}.

140 ↗(On Diff #47148)

Add double-quotes around $BLACKLISTDESTDIR/$BFILE

142 ↗(On Diff #47148)

Add double-quotes around $BLACKLISTDESTDIR/$BFILE

152 ↗(On Diff #47148)

Add double-quotes around $BLACKLISTDESTDIR

156 ↗(On Diff #47148)

Usage is supposed to be printed on stderr.
Add the following line before this first echo:

exec >&2

This will cause all following output to be redirected to stderr.

171 ↗(On Diff #47148)

Add double-quotes around $1

172–174 ↗(On Diff #47148)

Combine to form:

list) cmd_list ;;
172–189 ↗(On Diff #47148)

case patterns should be indented to the same level as the case and esac lines.

175–177 ↗(On Diff #47148)

Combine to form:

rehash) cmd_rehash ;;
178–180 ↗(On Diff #47148)

Add double-quotes around $@ and combine lines to form:

blacklist) cmd_blacklist "$@"
181–183 ↗(On Diff #47148)

Add double-quotes around $@ and combine lines to form:

unblacklist) cmd_unblacklist "$@"
184–186 ↗(On Diff #47148)

Combine to form:

blacklisted) cmd_blacklisted ;;
187–189 ↗(On Diff #47148)

The fallback for a case statement does not need the ;; terminator.
Remove unnecessary terminator.
Combine lines and add comment to remind us that usage calls exit

*) usage # NOTREACHED
192–195 ↗(On Diff #47148)

This stanza destroys the exit status of the desired function. Consider the following solution:

retval=$?
[ $ERRORS -gt 0 ] && echo "Encountered $ERRORS errors" >&2
exit $retval
This revision now requires changes to proceed.Aug 23 2018, 8:57 PM

Corrections

usr.sbin/trustctl/trustctl.sh
178–180 ↗(On Diff #47148)

Correction:

blacklist) cmd_blacklist "$@" ;;
181–183 ↗(On Diff #47148)

Correction:

unblacklist) cmd_unblacklist "$@" ;;

Template suggestion because file is pretty large

usr.sbin/trustctl/trustctl.sh
31–41 ↗(On Diff #47148)

You have enough globals, functions, and sundry that it's worth templatizing this file.

The template structure is simple.

# x 60 + single space + all-caps description of section

And it has a 3-line footer:

# x 80
# END
# x 80

Here you would put:

############################################################ CONFIGURATION

: ${SEARCHPATH:=...
...

############################################################ GLOBALS

SCRIPTNAME=...
...

############################################################ FUNCTIONS

do_hash()
{
...

And then after the last function:

############################################################ MAIN
allanjude marked 59 inline comments as done.Aug 23 2018, 10:27 PM

Thanks for all of the feedback

usr.sbin/trustctl/trustctl.sh
72–75 ↗(On Diff #47148)

In this case $1 is the name of a function, we should still double quote that?

allanjude marked an inline comment as done.Aug 23 2018, 10:28 PM
allanjude updated this revision to Diff 47211.

Updated with extensive feedback from dteske@

dteske added inline comments.Aug 29 2018, 6:59 AM
usr.sbin/trustctl/trustctl.sh
31–32 ↗(On Diff #47148)

BLACKLISTPATH and TRUSTPATH are now colon-delimited. Need to update these defaults

72–75 ↗(On Diff #47148)

Yes. Prevents word splitting, so that you get expected results should someone try to overload the keyword in the scripts arguments

103 ↗(On Diff #47148)

Don't forget to mark this as done

30 ↗(On Diff #47211)

Add blank line after "#x60 CONFIGURATION" and before the first variable. This makes it easier to jump to the top of the variables using "{" and "}" paragraph navigation in nvi/vi/vim (if you use those editors) or something comparable in other editors.

37 ↗(On Diff #47211)

Add blank line after "#x60 GLOBALS"

38 ↗(On Diff #47211)

Add double-quotes around ${0##*/}

42 ↗(On Diff #47211)

Blank line after "#x60 FUNCTIONS"

204 ↗(On Diff #47211)

Add a blank line after "#x60 MAIN"

jhb added a subscriber: jhb.Aug 29 2018, 5:55 PM

A bit of a meta comment: "trustctl" seems like a very generic name. This tool is managing the SSL certificate bundle, not handling various types of trust databases in some generic way. As such, I'd give it a more specific name like 'sslcertctl'.

allanjude marked 8 inline comments as done.Aug 31 2018, 2:46 AM
allanjude updated this revision to Diff 47515.Aug 31 2018, 2:47 AM

Rename trustctl to certctl

Address additional feedback from dteske

allanjude retitled this revision from Introduce trustctl(8) to Introduce certctl(8).Aug 31 2018, 2:47 AM

The patch is showing usr.sbin/certctl/trustctl.sh -- is this a phab issue or did the file not get renamed in the patch?

dteske added inline comments.Aug 31 2018, 5:06 AM
usr.sbin/certctl/Makefile
4

Rename to certctl.sh ?

allanjude updated this revision to Diff 47805.Sep 8 2018, 3:42 AM

Finish renaming things

Minor nit: could you please quote variables with {} (e.g. $VAR -> ${VAR})?

usr.sbin/Makefile
11

Could you please make this dependent on MK_OPENSSL because the utility requires it?

usr.sbin/certctl/certctl.sh
37

(Not a blocking issue) Although unlikely to happen in practice, it's probably a good idea to not assume there would be no collision.

80

Why is this copied (instead of a symbolic link)?

128

Minor nit: 127-134 and 136-143 are similar enough to be abstracted into a helper routine instead of duplicating most of the code. It would make the code easier to read as well.

145

Arguably, I would expect the hash symbolic links be removed when the certificate was for any reason, gone. The current code in do_list would issue a warning, but would not take any action.

200

Minor nit: you are rehashing both blacklisted and trusted certificates.

I don't think OpenSSL actually supports blacklists, were these for this script only?

dteske added inline comments.Sep 11 2018, 10:36 PM
usr.sbin/certctl/certctl.sh
37

Allan quoted the globs here so they won't be expanded until CWD is a directory where those extensions are expected to be found. What might "collision" mean in this context?

128

We can't boil it down to (where x is whatever name you give the function`):

x "$BLACKLISTPATH"
x "$CPATH"

Because the do_scan and echo arguments are different.

Reducing it down to the unique elements:

x "$BLACKLISTPATH" blacklisted create_blacklisted
x "$TRUSTPATH" trusted create_trusted_link

Doesn't appear to improve readability for me considering it doesn't tell you that the first argument is a colon-separated list of directories, second argument is part of a string that is echo'd, and that the last argument is a argument to the do_scan function.

Alternatives could be to add flags:

x -p "$BLACKLISTPATH" -t blacklisted -f create_blacklisted
x -p "$TRUSTPATH" -t trusted -f create_trusted_link

That's getting better. Maybe if we get the rest of the way there by using a good function name and longer arguments:

scanpath -t "Scanning %s for blacklisted certificates..." \
        -f create_blacklisted "$BLACKLISTPATH"
scanpath -t "Scanning %s for trusted certificates..." \
        -f create_trusted_link "$TRUSTPATH"

That looks pretty good.

But now we have to create the function to back that readable code. This could potentially impact the decision to refactor this code into a function. Let's take a look:

scanpath()
{
        local OPTIND=1 OPTARG flag
        local path text= func=
        while getopts t:f: flag; do
                case "$flag" in
                t) text="$OPTARG" ;;
                f) func="$OPTARG" ;;
                esac
        done
        shift $(( $OPTIND - 1 ))
        local oldIFS="$IFS" IFS=:
        set -- "$1"
        IFS="$oldIFS"
        for path in "$@"; do
                [ -d "$path" ] || continue
                [ "$text" ] && printf "$text\n" "$path"
                do_scan "$func" "$path"
        done
}

The function is 20 lines. Adding the calls (4 lines), thats 24 lines total.

Without the function, the two code segments mentioned amount to 16 lines of code.

So I throw it back at Allan (owner of the code and ultimate decision maker) and Xin (suggester of refactor) to decide.

Making it a function could cause the code to increase by 8 lines of code; but maybe the result is desirable.

allanjude marked 8 inline comments as done.Oct 3 2018, 2:58 AM
allanjude updated this revision to Diff 48668.

Address feedback from delphij

allanjude updated this revision to Diff 48669.Oct 3 2018, 2:59 AM

Add missing manpage

allanjude marked 2 inline comments as done.Oct 3 2018, 3:00 AM
allanjude added inline comments.
usr.sbin/certctl/certctl.sh
80

This was to deal with the fact that the user might have the 'bad' certificate in an ephemeral location, but that is their problem, so I switched it to symlink.

128

I was able to refactor this in a way I think looks nicer.

145

list is a read only command, so should not make changes. Doing a rehash would remove any missing certificates.

200

Yes, the blacklist just prevents a certificate from getting hashed into the trust store by certctl

imp added a comment.Oct 3 2018, 2:48 PM

Note: I've not reviewed the shell script itself...

usr.sbin/Makefile
127

Why not SUBDIR.${MK_OPENSSL}+= certctl here?

allanjude updated this revision to Diff 48673.Oct 3 2018, 2:58 PM

Fix makefile

0mp requested changes to this revision.Oct 5 2018, 9:53 AM
0mp added a subscriber: 0mp.

igor and mandoc -Tlint report some problems with the manual page.

usr.sbin/certctl/certctl.8
110

Do we want to cross reference this new manpage from existing ones (like openssl(1))?

This revision now requires changes to proceed.Oct 5 2018, 9:53 AM
kevans added a subscriber: kevans.Jan 13 2019, 5:12 AM
kevans commandeered this revision.
kevans updated this revision to Diff 54108.Feb 20 2019, 2:29 AM

Main changes:

  • Gate certctl build/installation on MK_CAROOT && MK_OPENSSL
  • Sprinkle some DESTDIR around
bcr accepted this revision as: manpages, bcr.Apr 4 2019, 8:59 AM
bcr added a subscriber: bcr.

OK for the man page.
Regarding @0mp's comment about the reference from openssl(1): we can do that later after this change has been committed. It's not critical to do that with this change.

kevans marked an inline comment as done.Apr 4 2019, 12:54 PM