Skip to content

Implement RFC8910: captive portal dhcp options #28132

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
Merged
merged 9 commits into from
Jul 3, 2023
Merged

Implement RFC8910: captive portal dhcp options #28132

merged 9 commits into from
Jul 3, 2023

Conversation

rpigott
Copy link
Contributor

@rpigott rpigott commented Jun 22, 2023 •
edited
Loading

This adds support for recording the captive portal advertised by dhcp option 114. It can be retrieved from the Describe link json output.

I figured since we are dropping the DVE-2018-0001 kludge now might be a good time to improve captive portal support with DHCP option 114.

Doesn't appear to actually work atm though... the portal is not recorded in our state file despite option 114 appearing in the dhcp option list according to tcpdump.

@rpigott rpigott force-pushed the dhcp-captive-portal branch from 6aefa2e to 5a24919 Compare June 23, 2023 06:56
@poettering
Copy link
Member

This probably should declare that this is an implementation of RFC 8910, i.e. https://www.rfc-editor.org/rfc/rfc8910.html

poettering
poettering reviewed Jun 23, 2023
View reviewed changes
poettering
poettering requested changes Jun 23, 2023
View reviewed changes
Copy link
Member

@poettering poettering left a comment

Choose a reason for hiding this comment

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

would be good to implement the same for dhcp6 (which uses a different option nr though) at the same time. and the IPv6RA support would be nice too.

@poettering
Copy link
Member

also needs man page updates

@poettering poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jun 23, 2023
@rpigott
Copy link
Contributor Author

rpigott commented Jun 23, 2023

would be good to implement the same for dhcp6 (which uses a different option nr though) at the same time. and the IPv6RA support would be nice too.

Yeah I plan to add these.

@rpigott rpigott force-pushed the dhcp-captive-portal branch from 5a24919 to de00ae7 Compare June 25, 2023 01:22
@rpigott
Copy link
Contributor Author

rpigott commented Jun 25, 2023 •
edited
Loading

Ok the dhcp4 option actually works as intended now.

This probably should declare that this is an implementation of RFC 8910, i.e. https://www.rfc-editor.org/rfc/rfc8910.html

@poettering Sounds good, but how do you mean? Just in the commit message?

RFC 8910 also includes this idea where a portal uri that is exactly "urn:ietf:params:capport:unrestricted" means there is explicitly no captive portal, and clients can skip any kind of detection. Should we have some kind of captive portal state in the api or just let clients figure that one out?

@rpigott rpigott force-pushed the dhcp-captive-portal branch 3 times, most recently from 27632f8 to 8722018 Compare June 25, 2023 06:53
@rpigott rpigott force-pushed the dhcp-captive-portal branch from 8722018 to 30ae6fa Compare June 25, 2023 06:57
@rpigott
Copy link
Contributor Author

rpigott commented Jun 25, 2023

Okay, added dhcpv6 + ipv6ra options and updated systemd.network(5) to include all the UseCaptivePortal directives. Should be in a reviewable state now.

Interesting questions:

  1. Do we care if the network advertises different portals using some of the 3 options? rfc says they should not do that.
  2. Should we have some state to record whether or not the advertised portal is the special value "urn:ietf:params:capport:unrestricted"?
  3. Is it fine just having it in the Describe dump or maybe there should be a property that changes?

@rpigott rpigott changed the title WIP: dhcp-client: add UseCaptivePortal to dhcp options dhcp-client: add UseCaptivePortal to dhcp options Jun 25, 2023
@rpigott rpigott marked this pull request as ready for review June 25, 2023 07:05
@github-actions github-actions bot added the please-review PR is ready for (re-)review by a maintainer label Jun 25, 2023
@rpigott rpigott requested a review from poettering June 25, 2023 07:05
@github-actions github-actions bot removed the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jun 25, 2023
yuwata
yuwata requested changes Jun 25, 2023
View reviewed changes
@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jun 25, 2023
@yuwata
Copy link
Member

yuwata commented Jun 25, 2023

Please add a simple testcase in test/test-network/systemd-networkd-tests.py.

@rpigott rpigott force-pushed the dhcp-captive-portal branch from 30ae6fa to 33456eb Compare June 25, 2023 19:20
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jun 25, 2023
@rpigott rpigott force-pushed the dhcp-captive-portal branch 2 times, most recently from ceeed20 to 2ecb240 Compare June 25, 2023 21:29
@yuwata yuwata added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jun 29, 2023
@rpigott rpigott force-pushed the dhcp-captive-portal branch from 7493a9a to 86b282b Compare June 29, 2023 22:44
@github-actions github-actions bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jun 29, 2023
@rpigott rpigott force-pushed the dhcp-captive-portal branch from 86b282b to a1798eb Compare June 30, 2023 00:03
@rpigott
Copy link
Contributor Author

rpigott commented Jun 30, 2023 •
edited
Loading

Could you rearrange the commits?

Here is a version with the requested history. Wasn't sure if the options should still be separate, lmk if I should squash them. Also caught that ipv6ra option default had not been set.

N.B. I haven't tested the intermediate commits, other than that they compile without error.

rpigott added 7 commits July 2, 2023 01:13
@rpigott
ndisc: parse RFC8910 captive portal ipv6ra option
9747955
@rpigott
network: Introduce UseCaptivePortal DHCPv4 option
edb88a7 
Accepts a boolean. When enabled, UseCaptivePortal will request and
retain the captive portal configuration from the DHCP server.
@rpigott
network: Introduce UseCaptivePortal DHCPv6 option
a75feb5 
Acepts a boolean. When enabled requests and retains captive portal
configuration from the DHCPv6 server.
@rpigott
network: Introduce UseCaptivePortal IPv6RA option
d74c4ce 
Accepts a boolean. When enabled retains captive portal configuration
advertised by the router.
@rpigott
networkd: include captive portal information in link json description
8628267
@rpigott
networkctl: show captive portal configuration in link status
e469d2a
@rpigott
test-network: add tests for captive portal dhcp options
dbe960f
@rpigott rpigott force-pushed the dhcp-captive-portal branch from a1798eb to dbe960f Compare July 2, 2023 08:18
@rpigott
Copy link
Contributor Author

rpigott commented Jul 2, 2023

fixed whitespace mismatch in the test.

yuwata
yuwata approved these changes Jul 3, 2023
View reviewed changes
Copy link
Member

@yuwata yuwata left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@yuwata yuwata merged commit 86c2a76 into systemd:main Jul 3, 2023
@github-actions github-actions bot removed the please-review PR is ready for (re-)review by a maintainer label Jul 3, 2023
@rpigott rpigott deleted the dhcp-captive-portal branch July 3, 2023 06:07
@evverx evverx mentioned this pull request Jul 3, 2023
Closed
@evverx
Copy link
Member

evverx commented Jul 3, 2023

There is a double-free somewhere: #28229

@evverx
Copy link
Member

evverx commented Jul 3, 2023

As far as I can see that part isn't covered by any tests at all. It would probably make sense to cover it too just in case.

This was referenced Jul 3, 2023
Merged
Closed
@rpigott rpigott mentioned this pull request Jul 4, 2023
Closed
@yuwata yuwata mentioned this pull request Jul 6, 2023
Merged
@evverx evverx mentioned this pull request Jul 10, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yuwata yuwata yuwata approved these changes

@poettering poettering Awaiting requested review from poettering

Assignees
No one assigned
Labels
documentation network tests util-lib
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rpigott @poettering @yuwata @evverx