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

Consider to use shorter armor lines #1099

Closed
kaie opened this issue Apr 14, 2020 · 8 comments · Fixed by #1177
Closed

Consider to use shorter armor lines #1099

kaie opened this issue Apr 14, 2020 · 8 comments · Fixed by #1177
Assignees
Milestone

Comments

@kaie
Copy link
Contributor

kaie commented Apr 14, 2020

When exporting armored blocks, e.g. for public keys, RNP uses a fixed line length of 76 chars. I believe this is set in the following line:

third_party/rnp/src/librepgp/stream-armor.cpp:    param->llen = 76; /* must be multiple of 4 */

When attaching these in email, Thunderbird wraps it at position 73.

(Armored blocks produced by GnuPG wrap at position 64, and didn't cause Enigmail to wrap.)

It's not a big deal, it simply looks slightly ugly when looking at the message source (with alternating lines of 73 and 3 characters).

Not sure if I should ask you to change it to 64, too, or make it configurable. What do you think?

Low priority, because from the functional perspective its working fine.

@ronaldtse
Copy link
Contributor

@kaie a shorter line length seems reasonable. No preference between 64 or 72...

Thoughts @ni4 @dewyatt ?

@ni4
Copy link
Contributor

ni4 commented Apr 15, 2020

@kaie @ronaldtse It can be easily changed, the only problem I see is propagating the line length parameter through the whole API.
The easies solution I see now is adding this parameter to rnp_output_to_armor().

@kaie
Copy link
Contributor Author

kaie commented Apr 15, 2020

Just FYI, I tested and can confirm that changing the hardcoded value in the line mentioned above is sufficient to change the line length.

@rrrooommmaaa
Copy link
Contributor

@ni4 Did you mean to configure it via rnp_cfg_t? And what value should be default?

@ni4
Copy link
Contributor

ni4 commented May 9, 2020

@rrrooommmaaa No, rnp_cfg_t is used only in CLI, while this should be changed on FFI layer.
Thing which comes first in mind is to update rnp_output_to_armor() function so type parameter will be allowed to have appended line length after the semicolon, i.e. type = message:64. As per RFC 4880-bis, maximum (and currently used as default) value is 76.

Also init_armored_src() should be updated with line length parameter (defaulting to current 76, which should be moved to some #define).

@kaie
Copy link
Contributor Author

kaie commented May 9, 2020

Not sure if you like this indirection, but here's another potential approach:

rnp_output_armor_set_line_length(rnp_output_t out_armor, size_t len);

You could define that calling rnp_output_armor_set_line_length is allowed, only, if parameter out_armor has been obtained from rnp_output_to_armor.

@kaie
Copy link
Contributor Author

kaie commented May 9, 2020

And maybe allow it to be set for the output object given to rnp_enarmor

@ni4
Copy link
Contributor

ni4 commented May 9, 2020

@kaie Agree, rnp_output_armor_set_line_length() could be also a good solution. It would also allow to extend functionality later on with other settings (like, in case we'll need to add some custom header or so on).

However not sure whether this possible for rnp_enarmor, since there output may be of any type, and all work is done in one pass. Probably, this should be better replaced with combination of rnp_output_to_armor() and function like rnp_input_to_output().

@ni4 ni4 added this to the v0.14.0 milestone May 12, 2020
rrrooommmaaa added a commit that referenced this issue Jun 20, 2020
rrrooommmaaa added a commit that referenced this issue Jun 21, 2020
rrrooommmaaa added a commit that referenced this issue Jun 21, 2020
ni4 pushed a commit that referenced this issue Jan 4, 2021
ni4 pushed a commit that referenced this issue Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants