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.

Sign up for GitHub

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

Jump to bottom

Mistyped flow rule values #126

Closed
altano opened this issue Jan 31, 2022 · 17 comments · Fixed by #186
Closed

Mistyped flow rule values #126

altano opened this issue Jan 31, 2022 · 17 comments · Fixed by #186

Comments

@altano
Copy link

altano commented Jan 31, 2022

On zeronsd startup I'm hitting this error:

ERROR - error syncing members: error in serde: invalid value: integer `4294967295`, expected i32 at line 1 column 5850

The rule I have causing this problem:

# Create a tag for which group someone is in
tag group
  id 1000
  default 0
  flag 0 productivity
  flag 1 homelab_mgmt
  flag 2 gaming
  flag 3 media
  enum 4294967295 everything    # max uint, catches all flags
;

In case it isn't clear, what I'm trying to accomplish is having a dropdown value that, when tand'd with any set of flags, is always positive. The rule that makes use of it:

# Drop any traffic between computers that don't share at least one group
break
  tand group 0
;

The ZeroTier Rules Engine documentation states:

[enum ] # value can be any 32-bit unsigned integer

So I believe something is mistyped in zeronsd and should be u32 instead of i32?

For now I am easily working around this by changing the enum to 2147483647, which is u31 effectively and fit's in a i32. (so it'll match flags 0-30, but not flag 31).
@erikh
Copy link
Contributor

erikh commented Jan 31, 2022 via email

@erikh
Copy link
Contributor

erikh commented Jan 31, 2022 via email

@erikh
Copy link
Contributor

erikh commented Feb 1, 2022 •
edited
Loading

So we looked into it today; OpenAPI has no way of expressing uint values at all, much less uint32! So this may be an ongoing issue using the API. For now, we're going to leave this ticket open and instruct people to not use the high bit of an unsigned int, which sucks as a request, but is necessary.

Sorry!

@altano
Copy link
Author

altano commented Feb 1, 2022

Sounds good. It should be trivial to workaround for most. This was easy to spot and zeronsd failed hard instead of silently processing the rules incorrectly (nice!), so it was easy to workaround. The status quo should be fine but here's some food for thought:

  1. If the API doesn't have a way of expressing the datatype, could you avoid the issue entirely by using an i64? It should be able to represent all values the rule engine requires? Since the rules are declarative I assume you don't have to replicate integer overflow behavior or anything?
  2. If there is an easy place to validate the current rules as a person is setting up zeronsd it might be nice to provide an error during setup, the same error that pops up during startup when the rules can't be parsed.

@erikh
Copy link
Contributor

erikh commented Feb 1, 2022 via email

@DePingus
Copy link

DePingus commented Apr 26, 2022 •
edited
Loading

Is this issue the same as:

Apr 26 02:14:01.489 ERROR zeronsd::authority: error syncing members: error in serde: invalid type: boolean `false`, expected i32 at line 1 column 5727
Apr 26 02:14:01.576 ERROR zeronsd::authority: error syncing members: error in serde: invalid type: boolean `false`, expected i32 at line 1 column 5727
Apr 26 02:14:31.475 ERROR zeronsd::authority: error syncing members: error in serde: invalid type: boolean `false`, expected i32 at line 1 column 5687
Apr 26 02:15:01.512 ERROR zeronsd::authority: error syncing members: error in serde: invalid type: boolean `false`, expected i32 at line 1 column 5687
Apr 26 02:15:31.479 ERROR zeronsd::authority: error syncing members: error in serde: invalid type: boolean `false`, expected i32 at line 1 column 5687
Apr 26 02:16:01.482 ERROR zeronsd::authority: error syncing members: error in serde: invalid type: boolean `false`, expected i32 at line 1 column 5717
Apr 26 02:16:31.479 ERROR zeronsd::authority: error syncing members: error in serde: invalid type: boolean `false`, expected i32 at line 1 column 5717
Apr 26 02:17:01.480 ERROR zeronsd::authority: error syncing members: error in serde: invalid type: boolean `false`, expected i32 at line 1 column 5718

I tested this on Debian and Alpine, both running in an unprivileged LXD container. I get the same errors on both. This is what my ruleset looks like:

# My Custom Ruleset

# Allow only IPv4, IPv4 ARP, and IPv6 Ethernet frames.
drop
	not ethertype ipv4
	and not ethertype arp
	and not ethertype ipv6
	and not chr ipauth
;

# Create a tag for which clientgrp someone is in
tag clientgrp
  id 1000
	default 100
  enum 100 user
  enum 200 server
  enum 800 dns
  enum 999 admin
;

# Allow all TCP and UDP packets (including SYN/!ACK) to these ports on dns clientgrp
accept
  dport 53
  and ipprotocol tcp or ipprotocol udp
	and treq clientgrp 800
;

# Allow all TCP packets (including SYN/!ACK) to these ports on servers clientgrp
accept
  dport 8096 or dport 80
  and ipprotocol tcp
	and treq clientgrp 200
;

# Drop TCP SYN,!ACK packets (new connections) not explicitly whitelisted above
break                     # break can be overridden by a capability
  chr tcp_syn             # TCP SYN (TCP flags will never match non-TCP packets)
  and not chr tcp_ack     # AND not TCP ACK
;

# Create a capability called superuser that lets its holders override all but the initial drop
cap superuser
  id 1000
  accept; # allow with no match conditions means allow anything and everything
;

# Accept anything else. This is required since default is drop.
accept;

@erikh
Copy link
Contributor

erikh commented Apr 26, 2022

Let me look into this, expect a result in the next few days. Please ping me if I do not get back to you!

@erikh
Copy link
Contributor

erikh commented Apr 26, 2022

Ok, this occurs when clientgrp is unset.

Let me look into how I can resolve this for you.

@erikh
Copy link
Contributor

erikh commented Apr 26, 2022

Ok, I have some bad news here and I'm really sorry. It might take longer to fix this than I had hoped; the tools we depend on don't accommodate for the type of work we need to do with openapi to account for this API.

This is a snippet from a modified openAPI that accommodates for the false value yielded from situations where the clientgrp isn't set. Central could change this behavior but may have reasons for doing otherwise.

          "tags": {
            "type": "array",
            "items": {
              "type": "array",
              "items": {
                "anyOf": [
                  {
                    "type": "integer"
                  },
                  {
                    "type": "boolean"
                  }
                ]
              }
            },

The original version looks more like this:

          "tags": {
            "type": "array",
            "items": {
              "type": "array",
              "items": {
                    "type": "integer"
              }
            },

which is why you're seeing what you're seeing. the false comes through in the API response and serde (the de-facto serialization library for rust, and should be used for this purpose) doesn't know what to do with a boolean. Rust is a strongly typed language so floating casting it to 0 or something like that would be a big no-no for serde.

So far this hasn't been too much of an issue, except for the previous issue that lead to this ticket. However, in your case it's the actual generator not generating appropriate code for anyOf, and instead just punting. Here's the struct out for both MemberConfig (where your tags lie) and the struct it refers to in MemberConfig, which was auto-generated:

#[derive(Clone, Debug, PartialEq, Default, Serialize, Deserialize)]
pub struct MemberConfig {
    /// Allow the member to be a bridge on the network
    #[serde(rename = "activeBridge", skip_serializing_if = "Option::is_none")]
    pub active_bridge: Option<bool>,
    /// Is the member authorized on the network
    #[serde(rename = "authorized", skip_serializing_if = "Option::is_none")]
    pub authorized: Option<bool>,
    #[serde(rename = "capabilities", skip_serializing_if = "Option::is_none")]
    pub capabilities: Option<Vec<i32>>,
    /// Time the member was created or first tried to join the network
    #[serde(rename = "creationTime", skip_serializing_if = "Option::is_none")]
    pub creation_time: Option<i64>,
    /// ID of the member node.  This is the 10 digit identifier that identifies a ZeroTier node.
    #[serde(rename = "id", skip_serializing_if = "Option::is_none")]
    pub id: Option<String>,
    /// Public Key of the member's Identity
    #[serde(rename = "identity", skip_serializing_if = "Option::is_none")]
    pub identity: Option<String>,
    /// List of assigned IP addresses
    #[serde(rename = "ipAssignments", skip_serializing_if = "Option::is_none")]
    pub ip_assignments: Option<Vec<String>>,
    /// Time the member was authorized on the network
    #[serde(rename = "lastAuthorizedTime", skip_serializing_if = "Option::is_none")]
    pub last_authorized_time: Option<i64>,
    /// Time the member was deauthorized on the network
    #[serde(rename = "lastDeauthorizedTime", skip_serializing_if = "Option::is_none")]
    pub last_deauthorized_time: Option<i64>,
    /// Exempt this member from the IP auto assignment pool on a Network
    #[serde(rename = "noAutoAssignIps", skip_serializing_if = "Option::is_none")]
    pub no_auto_assign_ips: Option<bool>,
    /// Member record revision count
    #[serde(rename = "revision", skip_serializing_if = "Option::is_none")]
    pub revision: Option<i32>,
    /// Array of 2 member tuples of tag [ID, tag value]
    #[serde(rename = "tags", skip_serializing_if = "Option::is_none")]
    pub tags: Option<Vec<Vec<crate::models::MemberConfigTagsItemsInner>>>,
    /// Major version of the client
    #[serde(rename = "vMajor", skip_serializing_if = "Option::is_none")]
    pub v_major: Option<i32>,
    /// Minor version of the client
    #[serde(rename = "vMinor", skip_serializing_if = "Option::is_none")]
    pub v_minor: Option<i32>,
    /// Revision number of the client
    #[serde(rename = "vRev", skip_serializing_if = "Option::is_none")]
    pub v_rev: Option<i32>,
    /// Protocol version of the client
    #[serde(rename = "vProto", skip_serializing_if = "Option::is_none")]
    pub v_proto: Option<i32>,
}


#[derive(Clone, Debug, PartialEq, Default, Serialize, Deserialize)]
pub struct MemberConfigTagsItemsInner {
}

If it's not obvious, the inner struct contains no actual... data... of any kind.

So I need to use a new openapi generator library. I'll source one, but this ticket may take some time. In the meantime, I would encourage you to make sure all your tag ids are named and that you have all of the members of your network assigned to a tag.

@erikh erikh mentioned this issue Apr 26, 2022
Open
6 tasks
@erikh
Copy link
Contributor

erikh commented Apr 26, 2022

There's a ticket open in the generator we use here already. I will keep an eye on this ticket and resolve this issue then.

I took a look at the current state of the art replacements and nothing really solves for openapi 3 yet, although paperclip may eventually. Not much I can do without a gigantic amount of engineering effort.

@DePingus
Copy link

On your suggestion I went into ZT Central and checked. I had one client that did not have a tag. Adding a tag to that client resolved the issue. Thanks!

@erikh
Copy link
Contributor

erikh commented Apr 27, 2022 via email

@erikh
Copy link
Contributor

erikh commented Apr 27, 2022

Ok; I have a fix for this by changing openapi libraries. Expect a new release in the next few days that resolves your issue.

@erikh erikh mentioned this issue Apr 29, 2022
Merged
@SorCelien
Copy link

SorCelien commented May 7, 2022 •
edited
Loading

Hello, I also had the error in serde error on my ZeroNSD because I had tags with false values. However these tags were tags that I had removed from my flow rules. Why does the controller leave tags on members when these tags have been deleted? The only way I found to remove these old tags was to reapply the only tag I use via an api post.

@erikh
Copy link
Contributor

erikh commented May 8, 2022 via email

@laduke
Copy link
Contributor

laduke commented May 8, 2022

I don't think there's a reason other than it would be a pain to fix.
The UI will delete defunct tags from members, like your api post, if you fiddle with them after changing the ruleset.

@erikh
Copy link
Contributor

erikh commented May 11, 2022

FWIW, This should all get resolved once oxidecomputer/progenitor#43 happens; once it's made into a proper package I've tested solutions that work fine with all the scenarios above best I can tell.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Progenitor port erikh/zeronsd
5 participants
@altano @laduke @erikh @DePingus @SorCelien