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

pci.4 + setkey.8: markup fixes #1476

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

concussious
Copy link
Contributor

@concussious concussious commented Oct 17, 2024

The ipsec setkey(8) manual can not be read on standard console, let alone anything narrower. Attempting to refactor the table starting at line 650 to make this possible.

This needs reviewed by someone who understands crypto.

Also some minor cleanups for pci(4) and setkey(8):

  • Tn is deprecated, zap it and reflow lines accordingly
  • xref mentioned device.hints(5) in pci(4)
  • racoon now comes from security/racoon2

Cc @gperciva @mhorne

+ Dl should be used instead of a display block for single lines
+ zap Tn and tweak line folding accordingly
+ massage line widths to style guide
+ there was a tab instead of a space causing linter errors
+ device.hints was mentioned, so xref it's manual

MFC after:	3 days
@concussious
Copy link
Contributor Author

Is there anything we can do to fix this table in setkey.8 at line 650?

@gperciva
Copy link
Contributor

gperciva commented Nov 2, 2024

Is there anything we can do to fix this table in setkey.8 at line 650?

First, I would remove the indent. (Or at least lower it to 2)

Second, the table uses tabs, but mdoc(7) discourages that. (Search it for "literal tabs" to find the paragraph.)

Third, if it used actual columns, we could save another 4 chars or so.

Fourth... well, the other 3 items might be enough. If you still want to shorten it, you could have a "multi-line header" and stick the "(bits)" below "keylen". Then we could reduce the width of that column by another 7 chars.

I haven't done the math regarding the 59-column view, but the above is more than enough to make it look great in an 80-column view.

+ refactor algo tables to fix on standard consoles
+ racoon now comes from ports/security/racoon2
+ align spacing, tag spdx, improve enclosure syntax

MFC after:	3 days
Reported by:	Graham Percival <[email protected]>
@concussious
Copy link
Contributor Author

concussious commented Nov 2, 2024

The whole page kinda doesn't work at 59, so, here's what I've got so far. Jump right to it in less with /hma.

tcp-md5 8 to 640 tcp: rfc2385
chacha20-poly1305 256 ah/esp: 128bit ICV (RFC7634)
.Ed
.Bl -column -offset indent "chacha20-poly130" "Key Size" "ICV" "Comment"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless you're an expert on cryptography, please don't change any text in this table. Especially when (after reducing the offset) it fits into the standard 80-char-wide terminal.

"keylen (bits)" is much more specific than "Key Size". If you moved "(bits)" onto the row below, I wouldn't complain (I suggested that, after all), but I don't recommending removing "bits" entirely.

The only text I'd feel comfortable removing are the "(no document)" labels.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not an expert in cryptography, and of course changing any text here would need someone who is to review even if I thought I was, but would you be willing to tell me more about this?

Googling, everything I found for keylen said it was an abreviation for key length, also called key size, and only given in bits.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm extremely allergic to touching anything cryptographic.

Linguistically, I would say that "length" is a measurement of a 1-dimensional object; whereas "size" could be a measurement of a 2d or 3d object as well. ("What's the size of that rectangle?")

Technically... well, for example, looking at the document which describes sha256, NIST-FIPS 180-4 [1], there's some places where they use "size", and other places where they use "length". I'm not certain that there's a difference, but I'm also not certain that there isn't a difference. So I personally would refrain from changing any current use of those terms in FreeBSD.

[1] https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.180-4.pdf

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the offset is always a last resort for improving legibility. I really appreciate the warning and have the same reservation. It doesn't mean we shouldn't respectfully try, it just means we can't pull it off alone. If I was a comitter, I would not commit this (or anything which changes content e.g. bumps date) until a SME comes to help. Eventually, one will get bored and look at this.

@@ -686,18 +691,18 @@ include authentication and should not be
paired with a separate authentication algorithm via
.Fl A .
.Ss Compression Algorithms
The following compression algorithms can be used
The following compression algorithm can be used
Copy link
Contributor

@gperciva gperciva Nov 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous text was correct: algorithms, since it's plural.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there's only one option?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I apologize, I looked at this way too quickly.

It's a weird subsection. Commit e6dede1 added a .Ss with a plural, and shortened the previous text (and kept the existing plural in that text). The very first commit 9a4365d had two compression algorithms, deflate and lzs.

Commit cf43a05 removed it from the man page, but not from the code (as far as I can see). Even today, token.l still contains lzs... along with oui (which I haven't heard of).

I am suspicious that the man page is inaccurate, but this is far from my area of expertise, so I withdraw my objection to this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow that adds a lot of context, thank you! Hopefully someone who uses this will come chime in. If they're in the code and work, I'd love to get them in here.

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