certctl: Restore distbase

[des]

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")

[des]

iterate

[des]

doc fixes

[markj]

usr.sbin/certctl/certctl.c

969

end might be pointing into an environment variable buffer, we should avoid modifying it.

[des]

don't modify environ

[jrtc27]

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]

usr.sbin/certctl/certctl.c

113

If the string doesn't have a trailing slash, won't this return an unterminated string?

[des]

usr.sbin/certctl/certctl.c

113

You're right, thank you.

183

That path is never used.

[des]

review feedback

[markj]

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]

more review feedback

[jrtc27]

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]

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]

further refinements

[cperciva]

Can this land now?

[des]

still waiting...

[cperciva]

@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]

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

[des]

usr.sbin/certctl/certctl.c

336

[markj]

usr.sbin/certctl/certctl.c

971

[des]

usr.sbin/certctl/certctl.c

971

On the contrary, DISTBASE _must_ begin with a slash.