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

Use 'IP' instead of raw 'ByteString' for AltNameIP constructor #102

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

KtorZ
Copy link

@KtorZ KtorZ commented Sep 25, 2018

This leaves the encoding / decoding of the IP address to the x509 library.
This encoding is quite specific for x509 and there's no reason why the
caller wouldn't like to leverage a higher-level type here.

Note, I've tested it in a different commit by exposing the ipFromBS and ipToBS constructors:

instance Arbitrary IP where
  arbitrary = oneof
    [ ipv4 <$> arbitrary <*> arbitrary <*> arbitrary <*> arbitrary
    , ipv6 <$> arbitrary <*> arbitrary <*> arbitrary <*> arbitrary <*> arbitrary <*> arbitrary <*> arbitrary <*> arbitrary
    ]

property_roundtrip_ip :: IP -> Bool
property_roundtrip_ip ip =
  case ipFromBS (ipToBS ip) of
    Left err -> error err
    Right ip' | ip' == ip -> True
              | otherwise -> error ("expected " ++ show ip ++ " got: " ++ show ip')
    ip:                   OK
      +++ OK, passed 100 tests.

Though, I am not sure you want to expose those as part of the main Data.X509 API (since Data.X509.Internal isn't exposed).

Let me know.

This leaves the encoding / decoding of the IP address to the x509 library.
This encoding is quite specific for x509 and there's no reason why the
caller wouldn't like to leverage a higher-level type here.
@KtorZ
Copy link
Author

KtorZ commented Sep 27, 2018

Switched from ip to foundation and defined a custom data IP to unify both Foundation.Network.IPv4 and Foundation.Network.IPv6 👍

@ocheron
Copy link
Contributor

ocheron commented Sep 27, 2018

I can understand the need for higher-level type but moving away from the ASN.1 definition also has downside. It increases the risk of parse failure.

If ever needed I implemented the reverse approach where content is kept as bytestring and parsed only in x509-validation.

Also in the Name Constraints extension, the same GeneralName datatype is used but with different content. The bytestring is the concatenation of address and mask.

@KtorZ
Copy link
Author

KtorZ commented Sep 27, 2018

Hey!
I am not sure to understand your point :thinking_face: ?

The encoding for IP addresses of x509 certs is well-defined through RFC so there's not much freedom given to developers when it comes to specifying an IP SAN.

This PR removes the hassle of doing the encoding ourselves.

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

Successfully merging this pull request may close these issues.

2 participants