bcgit / bc-csharp

BouncyCastle.NET Cryptography Library (Mirror)
https://www.bouncycastle.org/csharp
MIT License
1.67k stars 556 forks source link

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

Open boegi1 opened 6 months ago

boegi1 commented 6 months ago

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 commented 6 months ago

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

boegi1 commented 6 months ago

@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 commented 6 months ago

@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 commented 6 months ago

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

cipherboy commented 6 months ago

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 commented 5 months ago

I chose to add constructors with an addVersionHeader argument instead.