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

Allow removal of "Version" header in PGP encryption #533

Open
boegi1 opened this issue Apr 18, 2024 · 6 comments
Open

Allow removal of "Version" header in PGP encryption #533

boegi1 opened this issue Apr 18, 2024 · 6 comments

Comments

@boegi1
Copy link

boegi1 commented Apr 18, 2024

PGP encrypted armored files always contain the "Version" header, also after setting the header value to null (-> "Version: ").
Could you please change this, so that when setting the version header to null it's completely removed?

Proposed change (https://github.com/bcgit/bc-csharp/blob/master/crypto/src/bcpg/ArmoredOutputStream.cs):

Current implementation

public ArmoredOutputStream(Stream outStream, IDictionary<string, string> headers)
            : this(outStream)
        {
            foreach (var header in headers)
            {
                var headerList = new List<string>(1);
                headerList.Add(header.Value);
                m_headers[header.Key] = headerList;
            }
        }

Solution

public ArmoredOutputStream(Stream outStream, IDictionary<string, string> headers)
        : this(outStream)
    {
        foreach (var header in headers)
            SetHeader(header.Key, header.Value);
    }
@cipherboy
Copy link
Collaborator

@boegi1 Sorry if I'm missing something, but could call headers.Remove("Version") instead of setting it to null?

@boegi1
Copy link
Author

boegi1 commented Apr 18, 2024

@cipherboy My thoughts on this were that the SetHeader(header.Key, header.Value) function already contains the logic to remove a header where the value for a key is null. I also do not see any way to remove headers from the ArmoredOutputStream other than explicitly calling the SetHeader("Version", null) function. So wouldn't implicitly using it simplify it?
Please correct me if I miss something.

@cipherboy
Copy link
Collaborator

@boegi1 Right, I think this is fine for after-the-fact removal. But I'm curious about why you'd want to pass a dictionary with a header key (set to the null value) to the constructor in the first place, versus not including the header in the constructor's header dictionary at all, given you don't want them in the AOS? :-) My 2c.

@boegi1
Copy link
Author

boegi1 commented Apr 18, 2024

@cipherboy Doesn't passing an empty header Dictionary automatically add the version header?

@cipherboy
Copy link
Collaborator

Ah, so we do:

using Org.BouncyCastle.Bcpg;

var headers = new Dictionary<string, string>();
ArmoredOutputStream aos = new ArmoredOutputStream(Console.OpenStandardOutput(), headers);

aos.WriteByte(0x00);
aos.Close();

I think this change makes sense, but I'll double check with @peterdettman that he doesn't think it'll break anything. Thanks!

@peterdettman
Copy link
Collaborator

I chose to add constructors with an addVersionHeader argument instead.

hubot pushed a commit that referenced this issue May 22, 2024
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

No branches or pull requests

3 participants