@cfergeau can you please help getting the PR reviewed?

@cfergeau can you please help getting the PR reviewed?

I must admit I’m not familiar at all with link local addresses and IMDSv2, and I struggle to understand what #7 (f4a40d2) was doing, and what is different in this PR.

Does a nitro enclave have network related differences compared to a normal system/VM which would explain why it needs to be special cased in gvisor-tap-vsock? Or on the contrary, is it the change added for coreos which might be incorrect?

Hey @cfergeau,

yes AWS Nitro Enclaves per default do not have any networking besides the vsock connection between the EC2 parent instance and itself. (Please see link for more information)
Using gvisor-tap-vsock is one way how a fully routed tap0 based network interface can be added to the enclave including DNS support if required. Having an ec2-metadata-access feature flag based approach is beneficial for this use-case so that the enclave developer/operator can decide if the enclave should be able to communicate with the EC2 IMDSv2 service and is thus able to get credentials on its own or if these credentials should explicitly be passed from outside.
Unfortunately I cannot comment on the CoreOS related changes in #7 but as stated before, we are not changing any of the default behavior here but just add and option to explicitly allow access to the metadata service for AWS Nitro Enclave related use-cases.

@cfergeau @gbraad any update on the PR?

@cfergeau @gbraad any update on the PR?

Quite busy with other stuff, and tricky PR, so it takes time :-/

Have you considered adding this to the UDP code for consistency?

diff --git a/pkg/services/forwarder/udp.go b/pkg/services/forwarder/udp.go
index c1c0cea0..79770c4e 100644
--- a/pkg/services/forwarder/udp.go
+++ b/pkg/services/forwarder/udp.go
@@ -14,11 +14,11 @@ import (
        "gvisor.dev/gvisor/pkg/waiter"
 )
 
-func UDP(s *stack.Stack, nat map[tcpip.Address]tcpip.Address, natLock *sync.Mutex) *udp.Forwarder {
+func UDP(s *stack.Stack, nat map[tcpip.Address]tcpip.Address, natLock *sync.Mutex, ec2MetadataAccess bool) *udp.Forwarder {
        return udp.NewForwarder(s, func(r *udp.ForwarderRequest) {
                localAddress := r.ID().LocalAddress
 
-               if linkLocal().Contains(localAddress) || localAddress == header.IPv4Broadcast {
+               if (!ec2MetadataAccess && linkLocal().Contains(localAddress)) || localAddress == header.IPv4Broadcast {
                        return
                }
 
diff --git a/pkg/virtualnetwork/services.go b/pkg/virtualnetwork/services.go
index 6c4155e0..57c5dafa 100644
--- a/pkg/virtualnetwork/services.go
+++ b/pkg/virtualnetwork/services.go
@@ -26,7 +26,7 @@ func addServices(configuration *types.Configuration, s *stack.Stack, ipPool *tap
 
        tcpForwarder := forwarder.TCP(s, translation, &natLock, configuration.Ec2MetadataAccess)
        s.SetTransportProtocolHandler(tcp.ProtocolNumber, tcpForwarder.HandlePacket)
-       udpForwarder := forwarder.UDP(s, translation, &natLock)
+       udpForwarder := forwarder.UDP(s, translation, &natLock, configuration.Ec2MetadataAccess)
        s.SetTransportProtocolHandler(udp.ProtocolNumber, udpForwarder.HandlePacket)
 
        dnsMux, err := dnsServer(configuration, s)

Apart from this, I have a hard time deciding if:

• this PR is correct, but the name of the option should be more generic
• or if link-local addresses really should not be forwarded the way you do it (for example after reading https://www.geeksforgeeks.org/link-local-address/ ). I’m wondering if the ARP probing described in this article will be working fine, or if we could sometimes end up with the same link-local address for the VM and the EC2 server
• or if the commit from Guillaume was incorrect, and if we should revert it

Since not much progress has been made in answering these questions, I’m tempted to go ahead with your change, the option naming makes it focused on a very specific usecase. When we have a better understanding of the problem/more time, we can rename the option, or deprecate it, … if we come up with a better solution. Would that work for you?

Thanks for your feedback @cfergeau,
was considering adding UDP for consistency reasons as well but since that IMDSv2 does not process UPD at all I limited it to TCP only and also called it out in the description of the command line flag. The intention is to really allow access for this very special and narrow use-case thus the naming.
Happy to go ahead and see if at a later point in time we need to make it more generic.

@cfergeau, just wondering - do you have an rough estimate when we can get the PR merged?

/lgtm
/approve

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfergeau, dpdornseifer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

@cfergeau, just wondering - do you have an rough estimate when we can get the PR merged?

Finally merged, May was a slow/busy month for me, so it took some time :)

thanks a lot @cfergeau, one last question - when do you expect the next release to be cut?

thanks a lot @cfergeau, one last question - when do you expect the next release to be cut?

I’ll try to make one early next week.

@cfergeau any update on the release schedule?

@cfergeau any update on the release schedule?

Just released 0.8.7

I removed the code which blocks link local access. According to the commit: f4a40d2, forwarding packets to link local address was blocked due to recurring error messages from fedora coreos trying to connect to 169.254.169.254:80. However, I don't see those error logs in gvproxy. Tested with fedora coreos, Fedora Cloud, and an old fedora image (Fedora-Cloud-Base-34-1.2).

If blocking access to the link local address by default is not necessary, maybe the --ec2-metadata-access flag can be removed in the next release.