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

Support for SPF #170

Open
tokcum opened this issue Feb 10, 2023 · 24 comments
Open

Support for SPF #170

tokcum opened this issue Feb 10, 2023 · 24 comments

Comments

@tokcum
Copy link

tokcum commented Feb 10, 2023

Hi,

I'm looking into adding support for SPF to domain. I've seen that TXT records are already supported. I think it would be great if domain could serialize and deserialize SPF records according to RFC7208.

I'm planning to create a tool to check SPF records. However, I think serializing / deserializing SPF records should be placed in domain.

Looking at the code structure of domain, I guess the approach would be to add a module rdata::rfc7208 which relies on rdata::rfc1035::Txt.

Let me know what you think.

@partim
Copy link
Member

partim commented Feb 10, 2023

That should be something to add, indeed.

I’m not sure where it should live – probably not in rdata directly as it is a use of TXT records not an record data type in itself. Might even be a top-level spf module behind a feature flag (we have tsig, so this wouldn’t be entirely unprecedented). I’ll have to think about that.

@tokcum
Copy link
Author

tokcum commented Feb 10, 2023

Ah, I see. I skimmed through the tsig module (its huge!) and found that struct Tsig is in rdata::rfc2845, which is relatively short. That makes sense as Tsig is its own record type, while SPF is not.

I'll do some first steps towards data struct(s) following RFC 7208 and keep you posted.

@tokcum
Copy link
Author

tokcum commented Feb 11, 2023

Reading the RFC 7208, I'm wondering, what do we expect from the SPF support in domain. Should it simply parse the SPF record according to the ABNF defined, i. e. without paying attention to the semantics defined in the RFC, or should it be aware of details and throw errors for records which are "malformed"?

As an example, lets consider this requirement from the RFC:

Mechanisms after "all" will never be tested. Mechanisms listed after "all" MUST be ignored.  
Any "redirect" modifier ([Section 6.1](https://www.rfc-editor.org/rfc/rfc7208#section-6.1)) MUST 
be ignored when there is an "all" mechanism in the record, regardless of the relative ordering 
of the terms.

Should domain parse the following records or fail because of the above requirement?

v=spf1 -all +mx -> parse because complies to ABNF in RFC 7208 or fail because there is a mechanism after all?

v=spf1 redirect=example.org -all -> parse because complies to ABNF in RFC 7208 or fail because redirect is combined with all?

My actual understanding is that domain should fully implement the ABNF in RFC 7208 but should not care about semantics, such as in the above requirement. However, the data structure implemented by domain to deserialize SPF record must allow higher level libraries / tools to implement the SPF semantics.

@partim : What's your idea about that?

@tokcum
Copy link
Author

tokcum commented Feb 11, 2023

An SPF record might contain IPv4 / IPv6 addresses. We could use crate::base::net::{Ipv4Addr, Ipv6Addr} as a convenience for the lib user. However, domain supports for some of its functionality no_std. Should we try to make spf work with no_std as well?

@partim
Copy link
Member

partim commented Feb 12, 2023

I would indeed only want to have parsing/composing of the record data as part of domain. Parsing should probably be an iterator that returns terms. For composing, the usual strategy to provide a dedicated builder type atop an octets builder that has a push method is probably also good.

This should indeed by no_std capable if at all possible. Are you missing something from the IP address types in no-std mode?

@tokcum
Copy link
Author

tokcum commented Feb 12, 2023

Are you missing something from the IP address types in no-std mode?

I don't have much experience with no_std. I assume within domain, we can implement it in a way to not miss the IP address types from std. A user of domain who has std available should be able to parse to IP address within her code.

I also checked some crates handling IP Networks and CIDR notation: looks like they all rely on std, explicitly or implicitly.

So, I think the approach is to make this work on a low level, i. e. staying with no_std and without adding new dependencies to domain.

@partim
Copy link
Member

partim commented Feb 13, 2023

The idea of domain::base::net was to provide IpAddr and friends with everything that is possible in a no_std environment if std isn’t available. If it is, they are just re-exports from std. So, if you have std everything works as normal. If something doesn’t work in no_std and it doesn’t rely on things from std (or alloc), then we should add it. I don’t quite remember how thoroughly I stole, I mean: borrowed, things from std.

I suppose you need the FromStr impls. Those are there as I need them for the zonefile parser.

@tokcum
Copy link
Author

tokcum commented Feb 13, 2023

The idea of domain::base::net was to provide IpAddr and friends with everything that is possible in a no_std environment if std isn’t available. If it is, they are just re-exports from std. So, if you have std everything works as normal.

Thank you for pointing this out. I was not fully aware of this because I did not grasp the "cfg" statements in base::net. So, IpAddr types are available, with and without std. Exciting!

Thinking about CIDR (which is not available in std anyway), we could support it without additional dependencies by doing the math on our own. So, instead of introducing a new CIDR type,which would be specific to domain, we could just return two IP addresses.

Example: if we had 192.168.1.1/24 in the SPF record, we would return 192.168.1.1 and 192.168.0.0.

From the point of view of a library user, I would like to get the network address in this case and happy about getting it for free.

What do you think?

@tokcum
Copy link
Author

tokcum commented Feb 13, 2023

My current approach is to implement a Spf::from_record(record: Record<Name, Data>) -> Result<Spf, Error>. Record must be an Rtype:Txt and start with "v=spf1", otherwise from_records returns errors.

Today I tested very long records with > 250 chars to understand the implications. I found no problems, as domain provides an iterator.

Another thing is that [RFC1035], Sections 3.3 and 3.3.14, define that a single text DNS record can be composed of more than one string. I was not able to create such a record with my DNS provider and I've never seen such records. I assume, I've to setup a DNS server to test this.

The question is: do I have to take care of "multiple strings in a single TXT record" or is this done by the already available Txt implementation in domain? <- I've reviewed rdata::rfc1035 and I've got the impression that this is already covered. The TxtIterator I used for very long TXT records kicks in for multiple strings as well. Correct?

@partim
Copy link
Member

partim commented Feb 14, 2023

(We need threads ;) )

Thinking about CIDR

I would rather have a dedicated type in the spf module if there isn’t a commonly accepted standard type for it. This can be very simple, just a struct with two public attributes. I think this is better than making assumptions or having the user deduce what was there.

We have types for prefixes in the routecore crate, but I’m not convinced it should be a dependency for domain, even if optional.

My current approach is to implement a Spf::from_record(record: Record<Name, Data>) -> Result<Spf, Error>. Record must be an Rtype:Txt and start with "v=spf1", otherwise from_records returns errors.

Do you need the owner of the record? Otherwise, I would just base it on rtype::Txt directly and let the user make one of those. There are several ways to make one and this way all of them can be used.

Another thing is that [RFC1035], Sections 3.3 and 3.3.14, define that a single text DNS record can be composed of more than one string. I was not able to create such a record with my DNS provider and I've never seen such records. I assume, I've to setup a DNS server to test this.

I believe if you have a zone file like so:

example.com TXT "foo" "bar"

then you will end up with a TXT record that has two character string one with foo and one with bar. I think this is a very likely case with SPF, so we need to support it. I think you can assume that terms don’t cross character string boundaries, so I would probably implement an iterator that takes a CharStr<_> and returns terms as the core machinery.

Then you can very easily have a function that produces an iterator that essentially just flatmaps any iterator that returns CharStr<_> as its items (which is what Txt<_>::ter_char_strsdoes) using the above core iterator.

@tokcum
Copy link
Author

tokcum commented Feb 16, 2023

@partim Thank you for your support. I'm not sure to fully understand but I'm trying to think in that direction. I'll work on a first implementation. Let's see how it goes.

@tokcum
Copy link
Author

tokcum commented Feb 19, 2023

@partim With CharStr<_> do you mean CharStr<Octs> with Octs being a generic type?

I've another question regarding domain names. Spf record might contain domain names and I thought I might be using Dname for them. However, when I created a Dname from a slice containing "google.com", I got "BadLabel" error.

let domain = b"google.com".as_slice();
println!("Domain: {}", Dname::from_slice(domain).unwrap());

=> panicked at 'called Result::unwrap()on anErr value: BadLabel(Extended(103))'

I figure that Dname can only be used for domain names as part of DNS requests / replies? What type would you suggest to use for domain names?

@partim
Copy link
Member

partim commented Feb 20, 2023

@partim With CharStr<_> do you mean CharStr<Octs> with Octs being a generic type?

Correct.

I've another question regarding domain names. Spf record might contain domain names and I thought I might be using Dname for them. However, when I created a Dname from a slice containing "google.com", I got "BadLabel" error.

from_slice expects the data to be in wire format which differs from the presentation (i.e., human readable) format. You can’t convert between the two for free, so I suppose it’s best to keep the name as a string and leave it to the user to convert if needed.

@tokcum
Copy link
Author

tokcum commented Feb 25, 2023

keep the name as a string and leave it to the user to convert if needed

Ok, that's fine.

I guess we also keep macros as strings, e.g. %{d} which refers to the domain?!

So, basically domain::spf deserializes an SPF record from a TXT record with best effort syntax checking but without evaluating the different tokens. Or should we somehow identify the macros in the SPF record and provide them to the user for further investigation?

@partim
Copy link
Member

partim commented Feb 27, 2023

I want to say draw the line at tokens and leave their interpretation to external code? We probably shouldn’t include too much code that isn’t strictly DNS.

@Bas-Man
Copy link

Bas-Man commented Mar 2, 2023

Not sure if I should mention it. But I have an SPF deconstruct crate.

https://github.com/Bas-Man/rust-decon-spf

@partim
Copy link
Member

partim commented Mar 10, 2023

@Bas-Man You definitely should! Maybe this is all that @tokcum needs?

It seems your crate uses String to store the data. While that should work with domain and is likely good enough for most users, we are trying to be generic over how data is stored as much as possible. E.g., if you are using the bytes crate, you can parse the SPF data from a message without having to allocate at all. Would you be interested in supporting this sort of thing in decon-spf?

@Bas-Man
Copy link

Bas-Man commented Mar 10, 2023

I am. I might need some guidance regarding using bytes.
Also I have not looked at the code for a while. I am also looking to do some basic validation.

@partim
Copy link
Member

partim commented Mar 10, 2023

We are using the octseq for being generic over the type of octets sequence to be used. So instead of, say, just Spf that uses String, you’d have Spf<Octs> that uses whatever Octsis. There is aStrtype in _octseq_ that provides a string-like type atop any kind of octets sequence. It looks like all the strings are ASCII only – I should probably add anAscii` type to octseq since that is such a common type.

Ideally you whouldn’t use Vec<_> either but keep those portions in some sort of encoded form in an Octs, too. This would make it possible to use the crate in a no-std environment. But I think it’s fair to declare that out of scope and stick with vecs.

If you need some inspiration, perhaps have a look at domain’s rdata::svcb module. While SVCB uses a binary format, it also is a type/value kind-of thing, so a similar approach can work.

But, this is just how domain does things to stay open to more use cases. If you just want to switch out String for Bytes, that should be relatively easy, given that for ASCII data you mostly can get away with having [u8] rather than str.

@tokcum
Copy link
Author

tokcum commented Mar 11, 2023

Working on a no-std use case, was one of the reasons why I've chosen to contribute to domain. That's new to me and a great learning opportunity.

@partim, yes, the type I'm working on is Spf<Octs>.

Sorry, I'm not very fast in programming due to other obligations.

@Bas-Man, no worries, when you are faster then me, that's not a problem for me, as my motivation is learning first and public recognition second.

@Bas-Man
Copy link

Bas-Man commented Mar 11, 2023

My goal is also learning and contributing. Recognition does not enter into the equation. I am new to rust still to be honest.

@Bas-Man
Copy link

Bas-Man commented Mar 11, 2023

@tokcum Would it be possible to look at your code so that I can learn as well. I have not tried no_std.

@tokcum
Copy link
Author

tokcum commented Mar 11, 2023

@Bas-Man, welcome to the party. To be honest, I'm not even a professional developer. I'm in Security, that's why I love rust.

Sure. I'll beautify my code this weekend and let you know when I've updated my fork. For raising a proper PR I'll need more time, though.

@partim
Copy link
Member

partim commented Mar 11, 2023

@tokcum No rush! If you would like some early feedback, you can always open a PR and mark it as a draft.

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