Skip to content

add ip4config2 + http protocols support #1614

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Conversation

kraxel
Copy link
Contributor

@kraxel kraxel commented Apr 9, 2025

This adds http protocol support.

Usage example: see fn fetch_http in https://gitlab.com/kraxel/virt-firmware-rs/-/blob/main/efi-apps/src/bin/netboot.rs

Copy link
Member

@phip1611 phip1611 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution, looks good! I left a few remarks.

Further, before this can be merged: Please add a integration test to uefi-test-runner similar to our other protocol tests. To prevent flakiness, I'd say you try to get a HTTP Response from any of [google.com, example.com, amazon.com] and if any responds positive, the test succeeds.

I think we need a network device for the integration test. Not sure if we have one at the moment. Feel ree to reach out in case you need guidance.

@kraxel
Copy link
Contributor Author

kraxel commented Apr 9, 2025

Further, before this can be merged: Please add a integration test to uefi-test-runner similar to our other protocol tests. To prevent flakiness, I'd say you try to get a HTTP Response from any of [google.com, example.com, amazon.com] and if any responds positive, the test succeeds.

Hmm, I guess that calls for bringing in https://gitlab.com/kraxel/virt-firmware-rs/-/blob/main/efi-apps/src/net/ip4config2.rs first. That probably needs a few more refinements though, right now it simply uses println! to report progress on getting a DHCP lease, which I guess is not what you want ...

I think we need a network device for the integration test. Not sure if we have one at the moment. Feel ree to reach out in case you need guidance.

What is used for the tests? qemu I guess?

@kraxel
Copy link
Contributor Author

kraxel commented Apr 9, 2025

Hmm, I guess that calls for bringing in https://gitlab.com/kraxel/virt-firmware-rs/-/blob/main/efi-apps/src/net/ip4config2.rs first. That probably needs a few more refinements though, right now it simply uses println! to report progress on getting a DHCP lease, which I guess is not what you want ...

draft: #1615

@phip1611
Copy link
Member

phip1611 commented Apr 9, 2025

I think we need a network device for the integration test. Not sure if we have one at the moment. Feel ree to reach out in case you need guidance.

What is used for the tests? qemu I guess?

You can run the integration tests using cargo xtask run. This spawns a QEMU process, yes.

Here you can find an example for how a protocol integration test was added:

61bf575

@phip1611 phip1611 mentioned this pull request Apr 8, 2025
13 tasks
@kraxel kraxel mentioned this pull request Apr 10, 2025
@kraxel kraxel changed the title add http protocol support add ip4config2 + http protocols support Apr 10, 2025
status_code: HttpStatusCode::STATUS_UNSUPPORTED,
};

let mut body = vec![0; if expect_body { 16 * 1024 } else { 0 }];
Copy link
Member

Choose a reason for hiding this comment

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

16 * 1024? Where do these numbers come from?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

16k buffer for the body. Figured by experimenting a bit. I havn't seen the uefi protocol return more in one call. Often it is much less though, between 1000 + 1500 (payload of one ethernet frame?).

Copy link
Member

@phip1611 phip1611 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for working on this! I'll wait for a second review of Nicholas before we merge this.


// hard to find web sites which still allow plain http these days ...
info!("Testing HTTP");
let Some(_) = fetch_http(*h, "http://boot.netboot.xyz/robots.txt") else {
Copy link
Member

@nicholasbishop nicholasbishop Apr 14, 2025

Choose a reason for hiding this comment

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

How about http://example.com? (And https://example.com for below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, didn't now example.com actually exists.
Problem is that https connections to example.com do not work with current ovmf builds.
tianocore/edk2#10962

Copy link
Member

@nicholasbishop nicholasbishop Apr 15, 2025

Choose a reason for hiding this comment

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

Until ovmf works with https://example.com, how about example.com for http, and github.com for https? If github.com was inaccessible then CI probably wouldn't work anyway, so it seems like an OK website to rely on.

kraxel added 4 commits April 15, 2025 11:23
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
This is needed for https support (tls server certificate verification).

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
@kraxel
Copy link
Contributor Author

kraxel commented Apr 16, 2025

Ok, works locally but fails in CI, apparently the prebuilt binaries do not have TLS support included
(-DNETWORK_TLS_ENABLE=TRUE build option).

temporary stopgap, can be reverted once the prebuilt ovmf binarties
have https support included.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
@phip1611
Copy link
Member

The prebuilt OVMF files, you mean? I think we can fix that quite easily, can't we? @nicholasbishop

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