Skip to content
This repository has been archived by the owner on Jan 10, 2023. It is now read-only.

Fix metadata service block #178

Merged
merged 3 commits into from
Sep 26, 2018
Merged

Fix metadata service block #178

merged 3 commits into from
Sep 26, 2018

Conversation

sargun
Copy link
Contributor

@sargun sargun commented Sep 24, 2018

Description of the Change

This fixes a potential bypass allowing users to get to the real AWS metadata service.

@codecov
Copy link

codecov bot commented Sep 24, 2018

Codecov Report

Merging #178 into master will increase coverage by 10.48%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #178       +/-   ##
===========================================
+ Coverage   22.87%   33.36%   +10.48%     
===========================================
  Files          66       66               
  Lines        7876     7876               
===========================================
+ Hits         1802     2628      +826     
+ Misses       5861     4928      -933     
- Partials      213      320      +107
Impacted Files Coverage Δ
vpc/bpf/filter/filter.go 0% <0%> (ø) ⬆️
vpc/setup/setup_linux.go 0% <0%> (ø) ⬆️
filesystems/watcher.go 62% <0%> (+3.71%) ⬆️
config/config.go 97% <0%> (+4.5%) ⬆️
uploader/copy.go 50% <0%> (+5%) ⬆️
api/netflix/titus/agent.pb.go 29.31% <0%> (+7.56%) ⬆️
executor/runtime/docker/capabilities.go 88.88% <0%> (+11.11%) ⬆️
executor/runner/runner.go 60.62% <0%> (+11.87%) ⬆️
executor/runtime/container.go 91.3% <0%> (+13.04%) ⬆️
filesystems/xattr/degrading_utils.go 75% <0%> (+13.63%) ⬆️
... and 9 more

@coveralls
Copy link

coveralls commented Sep 24, 2018

Pull Request Test Coverage Report for Build 2128

  • 0 of 8 (0.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 24.509%

Changes Missing Coverage Covered Lines Changed/Added Lines %
vpc/bpf/filter/filter.go 0 1 0.0%
vpc/setup/setup_linux.go 0 7 0.0%
Totals Coverage Status
Change from base Build 2123: 0.0%
Covered Lines: 2433
Relevant Lines: 9927

💛 - Coveralls

@sargun
Copy link
Contributor Author

sargun commented Sep 24, 2018

Waiting on @rgulewich's changes before this will be merged / pass.

vpc/bpf/filter.c Outdated
@@ -36,8 +36,10 @@ int classifier_egress_filter(struct __sk_buff *skb) {

if (eth->h_proto == __constant_htons(ETH_P_IP) && iph->daddr == __constant_htonl(METADATA_SERVICE_IP))
return TC_ACT_SHOT;

/* See the explanation behind this in allocation_network_linux.go */
Copy link
Contributor

Choose a reason for hiding this comment

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

that file does not exist anymore

if err != nil {
return err
}

return setupIFB(ctx, vpc.EgressIFB, "classifier_egress")
return setupIFB(ctx, vpc.EgressIFB, "classifier_egress", unix.ETH_P_ALL)
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems that for !unix.ETH_P_ALL, the bpf filter is a noop and will always do TC_ACT_OK, or are you relying on it to validate the L2 and L3 header lengths and formats?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That should never happen (we should never get non-ethernet traffic on the interface veth that goes into the ifb).

Copy link
Contributor

Choose a reason for hiding this comment

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

my bad, I meant !unix.ETH_P_IP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, except for the block rule, it passes through all IP traffic. The amount of non-IP traffic we receive should be minimal, but it is on my long-term TODO list (there's a JIRA for it), to make this capture and classify all container traffic, not just IPv4 traffic.

@sargun sargun merged commit 93c7ea8 into master Sep 26, 2018
@sargun sargun deleted the fix-metadata-service-block branch September 26, 2018 16:20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants