In D23230#771332, @rscheff wrote:

Interesting. I could find this right away: ECN++: Adding Explicit Congestion Notification (ECN) to TCP Control Packets

Here is the history

Current work in the IETF (draft-generalized-ecn, also known as ECN++)

Note that this also fixes a (should be) harmless bug in ossl(4) where
the counter was incorrectly treated as a 64-bit counter instead of a
32-bit counter in terms of wrapping when using a 12 byte nonce.
However, this required a single message (TLS record) longer than 64 *
(2^32 - 1) bytes (about 256 GB) to trigger.

The patch to ssl_engine_io.c is correct from an openssl perspective.
I'm not a ports committer, so technically I shouldn't approve the (trivial) Makefile update.

Perhaps this is another reminder that our heimdal is quite old and we might want to revitalize the effort to move it to a private library and update it.

In D27842#622724, @asomers wrote:

Yeah. The other problem with using assertions to document the locking requirements is that the assertions only work for VOP callers. They don't help VOP implementors, like fusefs, to get it right.

I find it interesting that we have such a long history of a LOCKS section in VOP man pages, given that mdoc(7) includes a list of conventional manual sections, with note that "[t]hese sections should be used unless it's bsolutely necessary that custom sections be used."

In D27668#618491, @bcr wrote:

@bjk: Can you comment on this? When I think "Kerberos and FreeBSD", you come to mind. ;-)

The mdoc is fine, so okay from manpages, but see inline comment.

You want to add -width jail to the .Bl that's giving a mandoc/lint error at present:

(I read this and have no comment or objection. Will try to finish thinking about the openssl code soon.)

I agree with Benedict -- the man page looks very good on first read!

In D25847#573331, @rpokala wrote:

As we are moving away from portsnap, ...

We are? What will be the preferred mechanism for updating /usr/ports?

I don't have particular insight into what various distros are doing for SSLv3_method/etc. specifically (my main exposure is into Debian), but I think we can learn quite a bit from the analogous handling of SSLv2_method() and its removal. Specifically, upstream OpenSSL had removed the methods outright around the 1.0.2g/1.0.2h timeframe but had to add them back as stubs that return NULL, to improve ABI compatibility. (This was, IIRC, before the timeframe when separate no-foo and no-foo-method configure options were available, so there wasn't much of a middlle ground available.) This predates the use of github and open pre-commit review, so we just have the commit messages for https://github.com/openssl/openssl/commit/13313856 and https://github.com/openssl/openssl/commit/fcedd2d69d to go from for understanding the motivations of the upstream changes.

Okay, please go ahead.

I expect I should be the one committing this?
But I will hold off for a bit in case someone wants to respond to the 'kldstat' remark.

Looks good, thanks for the final updates!

Thanks for the updates; they look good.
Just one more thing (stray .Ed) that I missed last time, sorry.
mandoc -Tlint can help find this sort of thing in the future.

Please remember to bump .Dd for the final version

I agree with Conrad (but it should be a separate review); TLS 1.0 and 1.1's days are numbered, and I should really get around to progressing https://tools.ietf.org/html/draft-ietf-tls-oldversions-deprecate-06

In D24341#537980, @jhb wrote:

In D24341#537467, @bjk wrote:

There's some more discussion ongoing at https://mailarchive.ietf.org/arch/msg/ipsec/IyE2dP17oKPFXeBgSs6hjfjXiIE/ which may end up indicating that gone_in(13) is more reasonable for us.

My take on reading the responses in that thread is that removing in 13 probably is fine. If it really is 1990's boxes doing 3des, then those probably aren't relevant peers for FreeBSD 13.0. Also, unlike the assumption of the one response that is more mixed, FreeBSD users are fairly capable at using older releases if necessary. Thanks for prodding them to get more detail.

There's some more discussion ongoing at https://mailarchive.ietf.org/arch/msg/ipsec/IyE2dP17oKPFXeBgSs6hjfjXiIE/ which may end up indicating that gone_in(13) is more reasonable for us.

In D24341#537336, @cem wrote:

In D24341#537325, @jhb wrote:

I did ping Ben off list about 3DES and he seemed to think 14 was more prudent than 13 for IPsec:

While I personally am happy to see the end of triple-DES in general (I
wrote RFC 8429, after all), it seems that in the IKE world it is not quite
as much of a no-brainer as one might think:
https://datatracker.ietf.org/meeting/104/materials/minutes-104-ipsecme-00
suggests that we cannot get rid of it quickly, due to use in telephony.
...

The link is devoid of any actual basis for that assertion. It's literally just "cannot get rid of 3des. used in telephony."

There are probably still some changes to make here, but not really anything that affects functionality, I think.
Sorry to have spotted some things this time that I apparently missed the first time around :(

Sorry this is later than expected.
-1 just for indentation, "Xr syslog 3", and nodefgroup description; the rest is optional.

I think there are several places where the error path needs to zero the length field in an output structure as well as freeing the pointer within.
Usually there is a helper function to do this in a concise fashion, though the prevailing style of any given file is not always to use it.
Also some stylistic comments that do not necessarily require action.

The man page should get a bunch more markup; off the top of my head,
.Xr for man page references,
.Pa for filesystem paths,
Li for literal variables and values...

These look like fine content changes, but double-check the indentation in line 1182 (i.e., tabs vs. spaces).

In D15350#350715, @0mp wrote:

In D15350#348437, @bjk wrote:

Can you remind me why the .Dq Li is supposed to be better for inline commands?

As fair as I know we tend to use Dq Li instead of Nm make Cm build-like for inline commands. This is the style we follow in make.conf.5, src.conf.5, mqueufs.5. The Nm-based formatting is used in rc.conf.5 and find.1 on the other hand.

AFAICT, Dq Li is a more popular way to format commands.

The argument which is in favor of Dq Li is that we tend to make commands in Examples sections literal. We very rarely add Nm-like formatting to examples. Hence for the sake of consistency I'm proposing this change from Nm ... to Dq Li. 😄

What do you think about it, @bjk?

Just a couple of grammar nits on the manpage.

Trying to limit myself to reviewing as manpages and not getting into the design itself...

Feel free to use Approved-by: bjk (doc committer)

Can you remind me why the .Dq Li is supposed to be better for inline commands?

Sorry to have missed this when it first came in; the mdoc could use some changes.

Some drive-by nits inline.

Just one nit, otherwise this looks good.

Looks good other than the potential build issue (which I did not test).

As discussed in https://github.com/heimdal/heimdal/issues/368, the k5login file can be used to restrict access, so
I do not think that ignoring EACCESS and EPERM is a change to be taken lightly.

Who's going to commit those?