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

network: introduce MACsec #12222

Merged
merged 12 commits into from Apr 12, 2019
Merged

network: introduce MACsec #12222

merged 12 commits into from Apr 12, 2019

Conversation

yuwata
Copy link
Member

@yuwata yuwata commented Apr 5, 2019

This includes a revised version of #12184, several extensions for that, and testcases.

cc @ssahani.

src/network/netdev/macsec.c Show resolved Hide resolved
r = parse_ip_port(rvalue, &port);
if (r < 0) {
log_syntax(unit, LOG_ERR, filename, line, r,
"Failed to parse port for secure channel identifier. Ignoring assignment: %s",
Copy link
Member

Choose a reason for hiding this comment

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

maybe say valid range here in the error message

Copy link
Member Author

Choose a reason for hiding this comment

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

We usually do not show valid range in the error message. Why this case is special?

return r;

r = safe_atou32(rvalue, a ? &a->sa.packet_number : &b->sa.packet_number);
if (r < 0) {
Copy link
Member

Choose a reason for hiding this comment

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

according to the man page addition further up zero is not an OK value here, this should hence be filztered out?

Copy link
Member Author

Choose a reason for hiding this comment

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

0 means unset. So, usually setting 0 is meaningless, but not invalid. I'd like to keep the current logic.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a mandatory value. Hence we should warn and verify in the verify function . Kernel will not accept this. See https://elixir.bootlin.com/linux/latest/source/drivers/net/macsec.c#L1645

Copy link
Member Author

@yuwata yuwata Apr 10, 2019

Choose a reason for hiding this comment

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

No no. When netlink message has the attribute, then it must be positive. Or, in other words, the attribute is optional. networkd does not send the attribute when packet number is 0. So, it is not necessary to check the value.

Copy link
Contributor

@ssahani ssahani Apr 10, 2019

Choose a reason for hiding this comment

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

But kernel will fail to create since it verifies 'MACSEC_SA_ATTR_PN is available . it not it fails there and if available then is this zero.

macsec-test: MACsec could not be add transmit secure association configurations: Invalid argument

Copy link
Member Author

@yuwata yuwata Apr 10, 2019

Choose a reason for hiding this comment

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

Ah, I notice that PN is mandatory for TXSA, but not for RXSA. I will add a check and warning. Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

src/network/netdev/macsec.c Outdated Show resolved Hide resolved
src/network/netdev/macsec.c Outdated Show resolved Hide resolved
src/network/netdev/macsec.c Outdated Show resolved Hide resolved
@poettering
Copy link
Member

also, Ci fails, macsec definitions are missing

@poettering poettering added ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 5, 2019
src/network/netdev/macsec.c Outdated Show resolved Hide resolved
@ssahani
Copy link
Contributor

ssahani commented Apr 5, 2019

Or may be not keep this way. There is no way to figure the last callback

@yuwata yuwata added needs-rebase and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Apr 10, 2019
@yuwata
Copy link
Member Author

yuwata commented Apr 10, 2019

@poettering and @ssahani Thank you for the comments. I've force-pushed a revised version. But missing_*.h update is not included, so some CIs should still fail. I'd like to finish #11298 before this PR.
Anyway PTAL.

@yuwata yuwata added dont-merge 💣 and removed ci-fails/needs-rework 🔥 Please rework this, the CI noticed an issue with the PR labels Apr 10, 2019
@yuwata yuwata force-pushed the macsec branch 2 times, most recently from 733ac98 to 6b5128c Compare April 10, 2019 17:08
yuwata and others added 9 commits April 12, 2019 10:10
When the flag is set, buffer is cleared on failure.
This is a continuation of 2432d09.
Similar to READ_FULL_FILE_UNBASE64, read data is decoded with
unhexmem().
MACsec is introduced since kernel-4.6. Let's support order kernels.
Media Access Control Security (MACsec) is an 802.1AE IEEE
industry-standard security technology that provides secure
communication for all traffic on Ethernet links.
MACsec provides point-to-point security on Ethernet links between
directly connected nodes and is capable of identifying and preventing
most security threats, including denial of service, intrusion,
man-in-the-middle, masquerading, passive wiretapping, and playback attacks.

Closes systemd#5754
@yuwata
Copy link
Member Author

yuwata commented Apr 12, 2019

Rebased. I hope now all CIs will be green. PTAL.

@yuwata yuwata added this to the v243 milestone Apr 12, 2019
@poettering poettering merged commit b51629a into systemd:master Apr 12, 2019
@yuwata yuwata deleted the macsec branch April 12, 2019 14:05
@ssahani
Copy link
Contributor

ssahani commented Apr 12, 2019

Yeh!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants