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

Implement 802.1x (EAP) in network settings #1597

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

jcronenberg
Copy link
Contributor

Problem

No option to configure 802.1x (EAP) for network connections.

Solution

Add ability to configure 802.1x in agama.
Note: For the certificates etc. NM supports 3 different formats. I decided to only implement the file format as that is IMO the most useful and should suffice.

Testing

  • Added a new unit test
  • Tested manually
sudo agama auth login && AGAMA_TOKEN=`sudo agama auth show`
curl -X POST http://localhost/api/network/connections \
    -H "Content-Type: application/json" \
    -H "Authorization: Bearer $AGAMA_TOKEN" \
    -d '{ "id": "ethernet-test", "method4": "auto", "method6": "auto", "ignore_auto_dns": false, "status": "down", "ieee-8021x": { "eap": [ "peap" ], "phase2_auth": "mschapv2", "identity": "test_user", "password": "test_pw", "client_cert": "/etc/NetworkManager/system-connections/cert.pem", "private_key": "/etc/NetworkManager/system-connections/key.pem", "private_key_password": "test_pw", "peap_version": "1", "peap_label": true } }'
curl -X POST http://localhost/api/network/system/apply \
    -H "Content-Type: application/json" \
    -H "Authorization: Bearer $AGAMA_TOKEN"
nmcli con show ethernet-test
# See 802-1x.* settings set.

@coveralls
Copy link

coveralls commented Sep 10, 2024

Coverage Status

coverage: 71.803% (-0.03%) from 71.833%
when pulling 64d672f on jcronenberg:8021x
into ff3cf07 on openSUSE:master.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks quite good. Just a few comments. Thanks!

@@ -231,6 +231,81 @@
}
}
}
},
"ieee-8021x": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be great to describe each element of this section. For instance, ca_cert is described but identity is not.

rust/agama-server/src/network/model.rs Show resolved Hide resolved
@@ -63,6 +63,36 @@ impl Default for BondSettings {
}
}

#[derive(Clone, Debug, Serialize, Deserialize)]
pub struct IEEE8021XSettings {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, write some description. It will appear in our OpenAPI documentation.

rust/agama-server/src/network/nm/dbus.rs Outdated Show resolved Hide resolved
let private_key: String = private_key
.get()
.iter()
.map(|u| *u.downcast_ref::<u8>().unwrap() as char)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If something goes wrong here, the whole service will crash.

@jcronenberg
Copy link
Contributor Author

@imobachgs Thanks for the review. I think I should have addressed all your comments now.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, thank you. Let's merge it.

@imobachgs imobachgs merged commit 8900663 into agama-project:master Sep 10, 2024
1 check passed
@jcronenberg jcronenberg deleted the 8021x branch September 10, 2024 16:14
@imobachgs imobachgs mentioned this pull request Sep 20, 2024
imobachgs added a commit that referenced this pull request Sep 20, 2024
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.

3 participants