[CO-389] Write properties for cardano-sl-x509 #3620

KtorZ · 2018-09-18T06:18:26Z

Description

As per comment in #3610 --> #3610 (comment)

Linked issue

[CO-379]

Type of change

🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
~~[ ] CHANGELOG entry has been added and is linked to the correct PR on GitHub.~~

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

Screenshots (if available)

This make sure that for any configuration, we actually generate valid certificates. Note that the property doesn't pass for we provide AltDNS that are IP addresses and this turns out to be invalid. The underlying v3 extensions only supports DNS names and we should make sure we properly throw when given an IP address

erikd · 2018-09-18T06:24:33Z

x509/test/Test/Cardano/X509/Configuration/Arbitrary.hs

+        <*> arbitraryBasicString
+        <*> oneof [pure Nothing, Just <$> arbitraryBasicString]
+
+    shrink _ = []


So you have implemented this in QuickCheck and there is no shrink. If this fails, its going to be a pain in the neck.

We'll keep this for now, but I really recommend that you have a look at HedgeHog.

Well, not really right? There are shrinkers defined when they make sense. Here, there's just no way to shrink a DirConfiguration. Nevertheless, even without shrinking, a DirConfiguration don't get really unmanageable 😅

That's actually why I have also explictely defined it as [], to make it clear that I haven't forgotten to define a shrink method for that type but that is just that this is the only sensible choice :)

And so far, failures render nicely 😛 (I got a few during development)

disassembler

I'd also add this to release.nix so hydra reports checks for it since it's no longer part of tools.

disassembler · 2018-09-18T14:14:25Z

x509/src/Cardano/X509/Configuration.hs

+parseAltName :: MonadThrow m => String -> m AltName
+parseAltName name =
+    case IP.decode (T.pack name) of
+        Just _  -> throwM (ErrInvalidAltName "IP was given as an AltName but it should be a DNS name")


This will break deadalus. Please leave the IP SAN parsing in.

Hmmm... Good catch @Disasm
But IP SAN aren't recognized as a valid alternative names by the x509 library we use. I can perhaps tweak the hook which validates the name...

Does Daedalus uses 127.0.0.1 to contact the server instead of localhost?

disassembler · 2018-09-18T14:17:34Z

x509/test/Test/Cardano/X509/Configuration/Arbitrary.hs

+instance Arbitrary (Invalid AltNames) where
+    arbitrary =
+        fmap (Invalid . mkAltNames) $ listOf1 $ frequency
+            [ (80, pure "localhost")


Does this mean localhost is an invalid altName as well now? That breaks wallet connectScripts if so.

No. The Invalid is just biaised to sometimes generate invalid configurations such that we can use it to trigger multiple sort of errors.

KtorZ · 2018-09-19T06:17:42Z

@disassembler

Okay, I've updated the implementation to:

Accept IPs as SAN (using the IPv4 and IPv6 encoding / decoding functions)
Properly validate IP SAN and DNS SAN. The former one just does a plain equality check and the latter relies on the default validation hook from x509-validation (which validates DNS SAN or fails)
Updated properties to reflect these changes
Have a new property which checks that unknown ServiceID (i.e. not defined as SAN) are actually rejected, for server certificates (we don't check the name in case of client certificates because it doesn't make sense).

Let me know 👍

Also: I am not sure whether I should add something to release.nix or anything. Note that this package is going to be used by cardano-sl-wallet-new (PR incoming) so it will get built anyway. Would tests run as part of the build automatically?

KtorZ · 2018-09-21T11:32:18Z

@disassembler
So, while using this on the "demo cluster" story, I noticed an issue with our TLS Manager with IP SANs. We basically need to have our custom name validator here as well, so I've added it:

https://github.com/input-output-hk/cardano-sl/pull/3620/files#diff-a879fd32cda407f37cf36a9459d628e8

e.g. - given a non-positive expiry days - given an unknown ServiceID

…-x509-as-library [CO-389] Write properties for cardano-sl-x509

…hk/KtorZ/CO-389/cardano-sl-x509-as-library [CO-389] Write properties for cardano-sl-x509

KtorZ requested a review from erikd September 18, 2018 06:18

erikd approved these changes Sep 18, 2018

View reviewed changes

disassembler suggested changes Sep 18, 2018

View reviewed changes

KtorZ force-pushed the KtorZ/CO-389/cardano-sl-x509-as-library branch from b9b3894 to ea5bc21 Compare September 19, 2018 06:11

KtorZ force-pushed the KtorZ/CO-389/cardano-sl-x509-as-library branch 2 times, most recently from e705dfd to 46afb5d Compare September 21, 2018 11:30

[CO-389] Fix x509 generation: throw when provided invalid config

88dc613

e.g. - given a non-positive expiry days - given an unknown ServiceID

KtorZ force-pushed the KtorZ/CO-389/cardano-sl-x509-as-library branch 2 times, most recently from 003ec4a to 91ef67e Compare September 21, 2018 12:01

[CO-389] Update pkgs/default.nix

cac524c

KtorZ force-pushed the KtorZ/CO-389/cardano-sl-x509-as-library branch from 91ef67e to cac524c Compare September 21, 2018 13:17

disassembler approved these changes Sep 21, 2018

View reviewed changes

KtorZ merged commit b4a083d into develop Sep 21, 2018

KtorZ deleted the KtorZ/CO-389/cardano-sl-x509-as-library branch September 21, 2018 22:10

KtorZ added a commit that referenced this pull request Nov 9, 2018

Merge pull request #3620 from input-output-hk/KtorZ/CO-389/cardano-sl…

9202fc8

…-x509-as-library [CO-389] Write properties for cardano-sl-x509

KtorZ added a commit to input-output-hk/cardano-wallet-legacy that referenced this pull request Nov 9, 2018

Merge pull request input-output-hk/cardano-sl#3620 from input-output-…

3ab6d39

…hk/KtorZ/CO-389/cardano-sl-x509-as-library [CO-389] Write properties for cardano-sl-x509

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CO-389] Write properties for cardano-sl-x509 #3620

[CO-389] Write properties for cardano-sl-x509 #3620

KtorZ commented Sep 18, 2018

erikd Sep 18, 2018

KtorZ Sep 18, 2018

KtorZ Sep 18, 2018

disassembler left a comment

disassembler Sep 18, 2018

KtorZ Sep 18, 2018

disassembler Sep 18, 2018

KtorZ Sep 18, 2018

KtorZ commented Sep 19, 2018

KtorZ commented Sep 21, 2018

[CO-389] Write properties for cardano-sl-x509 #3620

[CO-389] Write properties for cardano-sl-x509 #3620

Conversation

KtorZ commented Sep 18, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

erikd Sep 18, 2018

Choose a reason for hiding this comment

KtorZ Sep 18, 2018

Choose a reason for hiding this comment

KtorZ Sep 18, 2018

Choose a reason for hiding this comment

disassembler left a comment

Choose a reason for hiding this comment

disassembler Sep 18, 2018

Choose a reason for hiding this comment

KtorZ Sep 18, 2018

Choose a reason for hiding this comment

disassembler Sep 18, 2018

Choose a reason for hiding this comment

KtorZ Sep 18, 2018

Choose a reason for hiding this comment

KtorZ commented Sep 19, 2018

KtorZ commented Sep 21, 2018