Details

Reviewers

jrtc27
markj

Group Reviewers

srcmgr
security

Commits

rGc340ef28fd38: certctl: Reimplement in C

Summary

This turns out to be required to produce correct metalog output.

While here,

Don't rely on X509_NAME_oneline() if we can avoid it.
Fix metalog output to be relative to DESTDIR.
Rework the tests so they're agnostic to DISTBASE.
Account for a recent change in openssl storeutl.
Add several new tests.

Reported by: jrtc27, kp
Fixes: 81d8827ad875 ("certctl: Reimplement in C")

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 66248
Build 63131: arc lint + arc unit

Event Timeline

des created this revision.Thu, Aug 14, 10:19 AM

Herald added subscribers: ziaee, emaste, imp. · View Herald TranscriptThu, Aug 14, 10:19 AM

des requested review of this revision.Thu, Aug 14, 10:19 AM

Harbormaster completed remote builds in B66229: Diff 160330.Thu, Aug 14, 10:19 AM

iterate

Harbormaster completed remote builds in B66230: Diff 160332.Thu, Aug 14, 10:54 AM

des edited the summary of this revision. (Show Details)Thu, Aug 14, 10:55 AM

doc fixes

Harbormaster completed remote builds in B66231: Diff 160333.Thu, Aug 14, 10:57 AM

markj added a subscriber: markj.Thu, Aug 14, 3:18 PM

markj added inline comments.

usr.sbin/certctl/certctl.c
958	`end` might be pointing into an environment variable buffer, we should avoid modifying it.

don't modify environ

Harbormaster completed remote builds in B66238: Diff 160341.Thu, Aug 14, 3:49 PM

Fix metalog output to be relative to DESTDIR.

Is this referring to being ./foo/bar/baz rather than /foo/bar/baz? If so might be clearer as something more like:

Fix metalog output to be a relative path, with leading . component, relative to DESTDIR, rather than an absolute path within DESTDIR

Not sure about the wording for the last part though. Maybe you know of a better way to describe it. What gets produced today is an absolute path, but it's an absolute path whose root is DESTDIR, and so in some sense is "relative" to DESTDIR, just not an actual relative path.

usr.sbin/certctl/certctl.c
150	Not quite true, LOCALBASE doesn't include DISTBASE. Not clear to me whether it should or not, bit of a weird interaction that doesn't really make sense to combine, so I don't really care which way it goes (I just happened to not include it for LOCALBASE in my original commit), but the comment should be consistent with the implementation.
183	read_cert also uses this for cert->path, is that a problem?

markj added inline comments.Thu, Aug 14, 5:21 PM

usr.sbin/certctl/certctl.c
113	If the string doesn't have a trailing slash, won't this return an unterminated string?

des marked 3 inline comments as done.Thu, Aug 14, 8:26 PM

des added inline comments.

usr.sbin/certctl/certctl.c
113	You're right, thank you.
183	That path is never used.

des marked 2 inline comments as done.Thu, Aug 14, 8:26 PM

review feedback

Harbormaster completed remote builds in B66245: Diff 160368.Thu, Aug 14, 8:30 PM

Looks ok to me with the inline comment addressed.

usr.sbin/certctl/certctl.8
128	Presumably we should check this?
usr.sbin/certctl/certctl.c
111

des retitled this revision from certctl: Restore distbase. to certctl: Restore distbase.Fri, Aug 15, 12:15 AM

more review feedback

Harbormaster completed remote builds in B66248: Diff 160374.Fri, Aug 15, 12:16 AM

des marked 2 inline comments as done.Fri, Aug 15, 12:16 AM

jrtc27 added inline comments.Fri, Aug 15, 12:23 AM

usr.sbin/certctl/certctl.c
117–118	Will happen automatically thanks to the loop condition. And makes a difference, because otherwise dst doesn't get incremented, and you'll truncate paths that end in a /. In practice you probably don't expect that case, and it still means the same thing, but still.
183	You sure about that? It looks a lot like list and untrusted at least will.

des marked 3 inline comments as done.Fri, Aug 15, 7:21 AM

des added inline comments.

usr.sbin/certctl/certctl.c
117–118	Did you bother to read the comment at the top of the function?
183	Ah, you're right, but they only print the basename, so it makes no difference.

des marked 2 inline comments as done.Fri, Aug 15, 7:22 AM

des edited the summary of this revision. (Show Details)Fri, Aug 15, 11:02 AM

further refinements

Harbormaster completed remote builds in B66263: Diff 160397.Fri, Aug 15, 11:03 AM

Can this land now?

still waiting...

@des Please commit this ASAP. Any further issues can be resolved later. The tree has been broken for 65 hours now; if that hits 72 hours I'm going to back out 81d8827ad875 until the bits are ready.

cperciva mentioned this in rG31ac42b486eb: Revert certctl reimplementation and follow-ups.Sun, Aug 17, 12:27 AM

Please revert 31ac42b486ebb5609c94cfd8a9bec86e457f9b53 whenever you're ready to land this.

des added inline comments.Sun, Aug 17, 10:31 PM