Add AllowedSigners support #23

timweri · 2024-04-28T23:35:37Z

This adds a new API to parse AllowedSigner. The specification can be found here: https://man.openbsd.org/ssh-keygen.1#ALLOWED_SIGNERS

The parser rejects unsupported options instead of building a HashMap to store additional options.

timweri · 2024-04-28T23:39:47Z

src/ssh/allowed_signer.rs

+        for option in parts {
+            let option = option.to_lowercase();
+            let (key, value) = match option.split_once('=') {
+                Some(v) => v,
+                None => (option.as_str(), ""),
+            };
+            match key {
+                "cert-authority" => cert_authority = true,
+                "namespaces" => {
+                    if namespaces.is_some() {
+                        return Err(Error::InvalidFormat);
+                    }
+                    let namespaces_inner: Vec<&str> = value.trim_matches('"')
+                            .split(',')
+                            .collect();
+                    namespaces = Some(
+                        namespaces_inner.iter()
+                            .map(|s| s.to_string())
+                            .collect()
+                    );
+                },


TODO: Rework the option parsing logic:

Support spaces in double quotes. Not sure how this works for a namespace. Need to find the specs for namespace.

Might need to enforce a format for namespace.

The key used for signing is specified using the -f option and may refer to either a private key, or a public key with the private half available via ssh-agent(1). An additional signature namespace, used to prevent signature confusion across different domains of use (e.g. file signing vs email signing) must be provided via the -n flag. Namespaces are arbitrary strings, and may include: “file” for file signing, “email” for email signing. For custom uses, it is recommended to use names following a NAMESPACE@YOUR.DOMAIN pattern to generate unambiguous namespaces.

From https://man.openbsd.org/ssh-keygen.1

timweri · 2024-04-28T23:40:19Z

src/ssh/allowed_signer.rs

+        let principals = parts.pop_front().ok_or(Error::InvalidFormat)?;
+        let principals: Vec<&str> = principals.split(',').collect();
+        let principals = principals.iter().map(|s| s.to_string()).collect();


Do we enforce a format for principals?

Seems like this is not done in other APIs (cert.rs) so maybe we don't need this?

I think principals are pretty open. There might be characters which cannot be in principals (","?)

I'm not enforcing format on principals

obelisk · 2024-04-29T17:36:23Z

src/ssh/allowed_signer.rs

+/// Please refer to [ssh-keygen-1.ALLOWED_SIGNERS] for more details about the format.
+/// [ssh-keygen-1.ALLOWED_SIGNERS]: https://man.openbsd.org/ssh-keygen.1#ALLOWED_SIGNERS
+#[derive(Debug, PartialEq, Eq)]
+pub struct AllowedSigners {


You might just want to use a typedef here (I don't think there would ever be additional fields in this struct.

I can't implement foreign traits on typedef since typedef is a weak symlink. So I chose to wrap AllowedSigners(Vec<AllowedSigner>)

tests/allowed_signers/good_allowed_signers

obelisk

Few things around parsing

obelisk · 2024-05-03T21:36:15Z

src/ssh/allowed_signer.rs

+            }
+
+            // Remove everything from the first #
+            if let Some(line) = line.split('#').next() {


This will break for namespaces which contain pound

It was intended. But I checked and git's allowed_signers allows # in namespaces. I updated the parsing logic.

tests/allowed-signers.rs

obelisk · 2024-05-04T04:55:35Z

src/ssh/allowed_signer.rs

+        let (mut head, mut rest) = s.split_once(char::is_whitespace)
+            .ok_or(Error::InvalidAllowedSigner("missing key data".to_string()))?;
+
+        let principals: Vec<&str> = head.split(',').collect();


Since you just realloc as String below, you may as well make this Vec and get the allocations done in one step.

obelisk · 2024-05-04T04:56:48Z

src/ssh/allowed_signer.rs

+
+        let principals: Vec<&str> = head.split(',').collect();
+        if principals.iter().any(|p| p.is_empty()) {
+            return Err(Error::InvalidAllowedSigner("principal cannot be empty".to_string()));


I think you can have a sub enum to make this more clear and get rid of the static strings from here (they'll move to the std::fmt::Display of the new enum type)

src/ssh/allowed_signer.rs

timweri · 2024-05-06T05:40:12Z

Indicates that the key is valid for use at or after the specified timestamp, which may be a date or time in the YYYYMMDD[Z] or YYYYMMDDHHMM[SS][Z] formats. Dates and times will be interpreted in the current system time zone unless suffixed with a Z character, which causes them to be interpreted in the UTC time zone.

I just realized the timestamp specs is YYYYMMDD[HHMM[SS]][Z]. I can parse this format from scratch, but then I still need to validate if a date is valid (eg. 30 Feb). So I imported chrono. I'm also open to not validating the timestamp, and just returns the timestamp as is so we don't have to include another external crate.

Another problem is UTC vs Local timezone. Right now, I return the local timezone timestamp as i64. If it's UTC, I convert it to local timezone. Not sure how to test the UTC -> Local timezone conversion in the unittest since Local timezone is not a static timezone.

obelisk · 2024-05-06T17:34:42Z

src/ssh/allowed_signer.rs

+
+impl AllowedSignerSplitter {
+    /// Split the string by delimiters but keep the delimiters.
+    pub(in self) fn new(s: &str) -> Self {


Is this different from pub(self)

No. I changed it to pub(self)

src/ssh/allowed_signer.rs

obelisk · 2024-05-06T17:36:32Z

src/ssh/allowed_signer.rs

+        // If we don't see any double quote in the token, we greedily parse the token until the
+        // next whitespace.
+        let mut s = String::new();
+        while !self.buffer.is_empty()


You might be able to get rid of the the last().unwrap() by using a while let here

I've used while let where I can. I also tried to use if let to remove the unwrap but that made things look a little uglier since if let _ && cond is still unstable.

obelisk · 2024-05-06T17:37:12Z

src/ssh/allowed_signer.rs

+/// The timestamp can be enclosed by quotation marks.
+fn parse_timestamp(s: &str) -> Result<i64> {
+    let mut s = s.trim_matches('"');
+    println!("s: {}", s);


Rogue println

src/ssh/allowed_signer.rs

obelisk

Just a couple comments. Mostly I'm worried about panics in the splitter due to extensive use of [] and unwrap.

timweri · 2024-05-07T20:12:12Z

I've got rid of all the unwrap I can. The remaining ones are only initialization/conversion calls from constants that should not fail.

Anyway, could you approve the GHA workflow to run the tests?

timweri added 2 commits April 28, 2024 19:29

Add allowed_signers

0bc7611

Add comment

057d53b

timweri commented Apr 28, 2024

View reviewed changes

obelisk reviewed Apr 29, 2024

View reviewed changes

tests/allowed_signers/good_allowed_signers Show resolved Hide resolved

obelisk requested changes Apr 29, 2024

View reviewed changes

Address comments

cd8c80f

timweri requested a review from obelisk May 3, 2024 20:47

obelisk reviewed May 3, 2024

View reviewed changes

timweri added 2 commits May 3, 2024 21:13

Update parsing

f529293

Update namespaces parsing

b9b2e2e

timweri force-pushed the add-allowed-signers branch from 1487bc2 to b9b2e2e Compare May 4, 2024 02:32

timweri commented May 4, 2024

View reviewed changes

tests/allowed-signers.rs Show resolved Hide resolved

obelisk reviewed May 4, 2024

View reviewed changes

timweri added 3 commits May 5, 2024 22:03

Update parsing logic

77e9be3

Clean up

bbc2cb5

Fix timestamp parsing

31b1e14

timweri requested a review from obelisk May 6, 2024 05:41

Add comment

59bb199

obelisk reviewed May 6, 2024

View reviewed changes

src/ssh/allowed_signer.rs Show resolved Hide resolved

obelisk reviewed May 6, 2024

View reviewed changes

src/ssh/allowed_signer.rs Show resolved Hide resolved

obelisk reviewed May 6, 2024

View reviewed changes

timweri added 2 commits May 7, 2024 16:04

Fix unwrap

7833486

Change in self to self

bfa3feb

timweri requested a review from obelisk May 7, 2024 20:10

timweri added 3 commits May 7, 2024 16:27

Refactor

eee2dac

Remove all local timezone in tests

ab90d81

Remove useless pub(self)

e655af2

timweri force-pushed the add-allowed-signers branch 2 times, most recently from ea1f496 to e655af2 Compare May 8, 2024 23:10

timweri and others added 2 commits May 8, 2024 19:10

Merge branch 'main' into add-allowed-signers

1bc0163

Merge branch 'main' into add-allowed-signers

2447d5c

obelisk merged commit 2b5106e into obelisk:main May 9, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AllowedSigners support #23

Add AllowedSigners support #23

timweri commented Apr 28, 2024

timweri Apr 28, 2024

timweri May 2, 2024

timweri Apr 28, 2024

timweri Apr 28, 2024

obelisk Apr 29, 2024

timweri May 3, 2024

obelisk Apr 29, 2024

timweri May 3, 2024

obelisk left a comment

obelisk May 3, 2024

timweri May 4, 2024

obelisk May 4, 2024

obelisk May 4, 2024

timweri May 6, 2024

timweri commented May 6, 2024

obelisk May 6, 2024

timweri May 7, 2024

obelisk May 6, 2024

timweri May 7, 2024

obelisk May 6, 2024

timweri May 7, 2024

obelisk left a comment

timweri commented May 7, 2024

Add AllowedSigners support #23

Add AllowedSigners support #23

Conversation

timweri commented Apr 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obelisk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timweri commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

obelisk left a comment

Choose a reason for hiding this comment

timweri commented May 7, 2024