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

Extend AssemblyAssertions with methods to check on assemblies being signed or not #2209

Closed
Corniel opened this issue May 23, 2023 · 9 comments · Fixed by #2207
Closed

Extend AssemblyAssertions with methods to check on assemblies being signed or not #2209

Corniel opened this issue May 23, 2023 · 9 comments · Fixed by #2207
Labels
api-approved API was approved, it can be implemented

Comments

@Corniel
Copy link
Contributor

Corniel commented May 23, 2023

Background and motivation

To ensure that the public key of an signed assembly does not accidentally changes, having an assertion .HavePublicKey(string) can help. For completeness/symmetry reasons adding .BeUnsigned() might be beneficial too.

API Proposal

public class AssemblyAssertions : ReferenceTypeAssertions<Assembly, AssemblyAssertions>
{
    public AndConstraint<AssemblyAssertions> NotHavePublicKey(string because = "", params object[] becauseArgs);

    public AndConstraint<AssemblyAssertions> HavePublicKey(string publicKey, string because = "", params object[] becauseArgs);
}

API Usage

var signed = typeof(AssemblyA.ClassA).Assembly;
signed.Should().HavePublicKey("sdfds");

var unsigned = typeof(AssemblyB.ClassB).Assembly;
unsigned.Should().NotHavePublicKey();

Alternative Designs

No response

Risks

No response

@Corniel Corniel added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label May 23, 2023
@jnyrup jnyrup linked a pull request May 29, 2023 that will close this issue
7 tasks
@jnyrup
Copy link
Member

jnyrup commented May 29, 2023

Thanks for opening this proposal.
Since I'm not well-versed in this area I spent some time getting more familiar with it, so here goes my first set of observations/comments.

"Signing" seems to be an overloaded term which can both refer to "strong-name signing" and "digital signature signing".
To tackle that potential confusion HavePublicKey and NotHavePublicKey() seems like the safer choices to me - in addition that also fits in to the design of having negated assertions Not-prefixed .

I fiddled around with different tools, and it seems they do not agree in what casing public keys should be printed.
So it might be easier for us, if we use byte[] instead of string for the expected public key.
This also opens up some doors in terms of overloading as we no longer can get overload confusion with the optional string because.

public AndConstraint<AssemblyAssertions> HavePublicKey(byte[] publicKey, string because = "", params object[] becauseArgs);

Should there be an overload that validates only if an assembly has a public key, but not its contents?
The public key is then included in the returned AndWhichConstraint for further assertions.

public AndWhichConstraint<AssemblyAssertions, byte[]> HavePublicKey(string because = "", params object[] becauseArgs);

@Corniel
Copy link
Contributor Author

Corniel commented May 30, 2023

NotHavePublicKey() is fine with me.

I disagree with providing a public key as byte array. Take my library (Qowaiv). It's public key is 0024000004800000940000000602000000240000525341310004000001000100ef35df58aa7fec73a11e70572e6b3791601006ef3fb1c6c1f1a402ba83bb2edc975c61e8a32d792edb864127f0d2c67eb7a64a9d3a0cdb0b1bb37ff2d0fcfd7990304623c044439d04dac49624cc6d7937581419d995c2689f9898ec09c941b3eb3cab8e4fc8f90b4ae5d45ab03d691d4d1f4b68450dad41fed46671376934b0. Providing that as an array is not doable. On top of that, there is no easy way to get the public key as an array, so when using this, you first have to convert your public key string to an array.

I see no use case for only checking that an assembly has a public key, as if it is in your control you should ensure that the key does never change, and if it is not in your control, there is nothing you can do any way. And if being signed of a dependent assembly is required, the compiler will warn you, or even fail to compile.

@jnyrup jnyrup changed the title [API Proposal]: Extend AssemblyAssertions with methods to check on assemblies being singed or not [API Proposal]: Extend AssemblyAssertions with methods to check on assemblies being signed or not May 31, 2023
@dennisdoomen
Copy link
Member

I disagree with providing a public key as byte array. Take my library (Qowaiv). It's public key is 0024000004800000940000000602000000240000525341310004000001000100ef35df58aa7fec73a11e70572e6b3791601006ef3fb1c6c1f1a402ba83bb2edc975c61e8a32d792edb864127f0d2c67eb7a64a9d3a0cdb0b1bb37ff2d0fcfd7990304623c044439d04dac49624cc6d7937581419d995c2689f9898ec09c941b3eb3cab8e4fc8f90b4ae5d45ab03d691d4d1f4b68450dad41fed46671376934b0. Providing that as an array is not doable. On top of that, there is no easy way to get the public key as an array, so when using this, you first have to convert your public key string to an array.

I actually agree with you @Corniel.

@jnyrup
Copy link
Member

jnyrup commented Jun 7, 2023

Since it seems we can use Encoding.ASCII to avoid any transformations of the key, I'm also fine with HavePublicKey(string publicKey.

To illustrate my point, see how I can construct a key starting with either a and A, this is the case-sensitivity I'd like FA to not have any opinions about.

using System.Reflection;
using System.Text;

var asm = Assembly.GetExecutingAssembly();
        // v
var str = "a024000004800000940000000602000000240000525341310004000001000100ef35df58aa7fec73a11e70572e6b3791601006ef3fb1c6c1f1a402ba83bb2edc975c61e8a32d792edb864127f0d2c67eb7a64a9d3a0cdb0b1bb37ff2d0fcfd7990304623c044439d04dac49624cc6d7937581419d995c2689f9898ec09c941b3eb3cab8e4fc8f90b4ae5d45ab03d691d4d1f4b68450dad41fed46671376934b0";
var key = Encoding.ASCII.GetBytes(str);
var asmName = asm.GetName();
asmName.SetPublicKey(key);
Console.WriteLine(Encoding.ASCII.GetString(asmName.GetPublicKey()));

    // v
str = "A024000004800000940000000602000000240000525341310004000001000100ef35df58aa7fec73a11e70572e6b3791601006ef3fb1c6c1f1a402ba83bb2edc975c61e8a32d792edb864127f0d2c67eb7a64a9d3a0cdb0b1bb37ff2d0fcfd7990304623c044439d04dac49624cc6d7937581419d995c2689f9898ec09c941b3eb3cab8e4fc8f90b4ae5d45ab03d691d4d1f4b68450dad41fed46671376934b0";
key = Encoding.ASCII.GetBytes(str);
asmName.SetPublicKey(key);
Console.WriteLine(Encoding.ASCII.GetString(asmName.GetPublicKey()));

output

v
a024000004800000940000000602000000240000525341310004000001000100ef35df58aa7fec73a11e70572e6b3791601006ef3fb1c6c1f1a402ba83bb2edc975c61e8a32d792edb864127f0d2c67eb7a64a9d3a0cdb0b1bb37ff2d0fcfd7990304623c044439d04dac49624cc6d7937581419d995c2689f9898ec09c941b3eb3cab8e4fc8f90b4ae5d45ab03d691d4d1f4b68450dad41fed46671376934b0
A024000004800000940000000602000000240000525341310004000001000100ef35df58aa7fec73a11e70572e6b3791601006ef3fb1c6c1f1a402ba83bb2edc975c61e8a32d792edb864127f0d2c67eb7a64a9d3a0cdb0b1bb37ff2d0fcfd7990304623c044439d04dac49624cc6d7937581419d995c2689f9898ec09c941b3eb3cab8e4fc8f90b4ae5d45ab03d691d4d1f4b68450dad41fed46671376934b0
^

@IT-VBFK
Copy link
Contributor

IT-VBFK commented Jun 7, 2023

I always thought, that a key is always meant to be case-insensitive. That's kinda weird...

@jnyrup
Copy link
Member

jnyrup commented Jun 7, 2023

I always thought, that a key is always meant to be case-insensitive. That's kinda weird...

You're completely right @IT-VBFK!
I had a revelation on my way home on the bike, I was messing up hex strings and ASCII strings 🤦

It's the string comparison of hex strings that should be case-insensitive.

@jnyrup jnyrup added api-approved API was approved, it can be implemented and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Jun 8, 2023
@jnyrup
Copy link
Member

jnyrup commented Jun 8, 2023

I've updated the proposal with the approved API.

@Corniel
Copy link
Contributor Author

Corniel commented Jun 8, 2023

I see the reasoning behind .NotHavePublicKey vs .HavePublicKey. That being said, from a test a specification perspective, someAssembly.Should().BeUnsigned() tells the story better than someAssembly.Should().NotHavePublicKey(). The latter hints to something like someAssembly.Should().NotHavePublicKey("InvalidKey").

For me .HavePublicKey(key) is short for .BeSignedWithPublicKey(key), or .BeSignedWith(key), hence my original proposal.

@dennisdoomen
Copy link
Member

In the context of an assembly, I do have to admit that I like BeUnsigned and BeSignedWithPublicKey better as well.

@dennisdoomen dennisdoomen changed the title [API Proposal]: Extend AssemblyAssertions with methods to check on assemblies being signed or not Extend AssemblyAssertions with methods to check on assemblies being signed or not Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved API was approved, it can be implemented
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants