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

aws duplicate zones #230

Closed
sstarcher opened this issue Jun 7, 2017 · 16 comments
Closed

aws duplicate zones #230

sstarcher opened this issue Jun 7, 2017 · 16 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Milestone

Comments

@sstarcher
Copy link
Contributor

In AWS you can create a private dns zone that has the same domain name as a public zone. So in my case I might have

Public

  • mysite.com

And I may have multiple VPCs that each have a private zone of the same name
Private

  • mysite.com -> private to VPC-A
  • mysite.com -> private to VPC-B

If I give external-dns that's running in VPC-A access only to the public mysite.com and the private mysite.com in VPC-A and use a domain-filter=mysite.com.

It attempts to list the hosted zones and it calls route53:ListResourceRecordSets, but once it encounters VPC-Bs mysite.com it responds with an error and no longer processes the other zones that it can actually reach.

time="2017-06-07T19:01:25Z" level=error msg="AccessDenied: User: arn:aws:sts::ACCCOUNT:assumed-role/sstarcher-worker-dns-role/cb6a49a4-sstarcher-worker-dns-role is not authorized to perform: route53:ListResourceRecordSets on resource: arn:aws:route53:::hostedzone/XXXXXXX
	status code: 403, request id: b4485da9-4bb3-11e7-84b0-XXXXXXX"

It would be helpful in this case if it ignored the zone it does not have access to, but continued on to the other zones. Currently the only work around I know of is to give it access to all of my zones, but someone with access to the cluster could create dns entries in zones that they should not be using.

@hjacobs
Copy link
Contributor

hjacobs commented Jun 8, 2017

@sstarcher proper handling of the "access denied" error from Route53 sounds like a good idea. Are you willing to do a PR?

@ideahitme
Copy link

@hjacobs @sstarcher before making a PR with adding error handling for this case, could you show the IAM policy you use for external dns. AFAIK you can restrict the route53:ListHostedZone action to exclude you private dns zone, and error should not popup.

@sstarcher
Copy link
Contributor Author

sstarcher commented Jun 8, 2017

When originally making this PR I used something similar to the first block below. I have since changed it to the second block. I don't consider it a large security issue to allow the pod to ListResourceRecordSets in a hosted zone since they can't modify the resources in this case, but it's likely not preferable. I did not test to see if a user created a pod with the second IAM profile with a zone the user is not allowed to changed if the code would hit that error case and bail out of the loop.

{
 "Version": "2012-10-17",
 "Statement": [
   {
     "Effect": "Allow",
     "Action": [
       "route53:ChangeResourceRecordSets",
       "route53:ListResourceRecordSets"
     ],
     "Resource": [
       "arn:aws:route53:::hostedzone/PRIVATE_ZONE",
       "arn:aws:route53:::hostedzone/PUBLIC_ZONE"
     ]
   },
   {
     "Effect": "Allow",
     "Action": [
       "route53:ListHostedZones"
     ],
     "Resource": [
       "*"
     ]
   }
 ]
}
{
 "Version": "2012-10-17",
 "Statement": [
   {
     "Effect": "Allow",
     "Action": [
       "route53:ChangeResourceRecordSets"
     ],
     "Resource": [
       "arn:aws:route53:::hostedzone/PRIVATE_ZONE,
       "arn:aws:route53:::hostedzone/PUBLIC_ZONE"
     ]
   },
   {
     "Effect": "Allow",
     "Action": [
       "route53:ListHostedZones",
       "route53:ListResourceRecordSets"
     ],
     "Resource": [
       "*"
     ]
   }
 ]
}

@ideahitme
Copy link

I don't consider it a large security issue to allow the pod to ListResourceRecordSets in a hosted zone since they can't modify the resources in this case, but it's likely not preferable.

Unfortunately, there is more to this than security issue. External DNS is not "smart" enough to understand if visible Hosted Zones/RRS can or cannot be modified. Which means that if you already created a record "foo.bar" in your private hosted zone via External DNS, it will always try to modify the record in this zone (and hence fail because of ChangeResourceRecordSets restriction)

I did not test to see if a user created a pod with the second IAM profile with a zone the user is not allowed to changed if the code would hit that error case and bail out of the loop.

I think second IAM policy you provided might fail with the same error, if External DNS will decide to place a record in the hosted zone it has no access to.

What I had in mind is the following:

{
 "Version": "2012-10-17",
 "Statement": [
   {
     "Effect": "Allow",
     "Action": [
       "route53:ListHostedZones",
       "route53:ListResourceRecordSets"
       "route53:ChangeResourceRecordSets"
     ],
     "Resource": [
       "arn:aws:route53:::hostedzone/PRIVATE_ZONE,
       "arn:aws:route53:::hostedzone/PUBLIC_ZONE"
     ]
   },
 ]
}

This way External DNS should not fail, as it will only see hosted zones and records it can modify.

@sstarcher
Copy link
Contributor Author

@ideahitme ListHostedZones does not support that type of restriction. With the above IAM profile you will get access denied with ListHostedZones.

@ideahitme
Copy link

@sstarcher I see, in this case we will probably have to have another mechanism for domain filtering, for example with Hosted Zone IDs. I would not really like to go with relying on errors to ensure the mechanism to create records in the correct zone, as long as it is possible

@sstarcher
Copy link
Contributor Author

From a user perspective as long as external-dns not having permissions to a specific zone does not cause other zones to fail I as a user would be satisfied using

{
 "Version": "2012-10-17",
 "Statement": [
   {
     "Effect": "Allow",
     "Action": [
       "route53:ChangeResourceRecordSets",
       "route53:ListResourceRecordSets"
     ],
     "Resource": [
       "arn:aws:route53:::hostedzone/PRIVATE_ZONE",
       "arn:aws:route53:::hostedzone/PUBLIC_ZONE"
     ]
   },
   {
     "Effect": "Allow",
     "Action": [
       "route53:ListHostedZones"
     ],
     "Resource": [
       "*"
     ]
   }
 ]
}

But as you pointed out if a user in the cluster attempts to create a record in a zone that external-dns does not have access to it will likely cause it to stop functioning.

@sstarcher
Copy link
Contributor Author

From a deployment perspective of external-dns. Having the user supply hosted IDs is fairly painful and is best avoided.

@ideahitme
Copy link

How about something like and making External DNS fetch the zone details via GetHostedZones ?

{
 "Version": "2012-10-17",
 "Statement": [
   {
     "Effect": "Allow",
     "Action": [
       "route53:GetHostedZones",
       "route53:ListResourceRecordSets"
       "route53:ChangeResourceRecordSets"
     ],
     "Resource": [
       "arn:aws:route53:::hostedzone/PRIVATE_ZONE,
       "arn:aws:route53:::hostedzone/PUBLIC_ZONE"
     ]
   },
   {
     "Effect": "Allow",
     "Action": [
       "route53:ListHostedZones"
     ],
     "Resource": [
       "*"
     ]
   }
 ]
}

@ideahitme
Copy link

@sstarcher I agree it is painful to list zone IDs in the deployment manifest, additionally using both external-dns configuration and IAM policy document does not seem like the best practice.

@sstarcher
Copy link
Contributor Author

@ideahitme the above that's very similar to what I have in reply

I don't believe the GetHostedZones will accomplish anything. If you drop GetHostedZones it's identical to what I had above.

So with that the workflow would be as follows currently.

  • ListHostedZones
  • ListResourceRecordSets - If this is denied the listing stops.

My recommendation would be to use the IAM profile like that, but instead of failing out when you can't do a ListResourceRecordSets the code should skip that zone and log a warning and continue on to the next zone.

@ideahitme
Copy link

ideahitme commented Jun 8, 2017

@sstarcher the difference is subtle. There are two scenarios:

  1. Listing records
  2. Changing records

Both require the list of hosted zones to operate on. It is intuitively easier to understand that the GetHostedZone call will verify that the zone should be included/excluded. It is easier to do from implementation point of view (but this of course should not be the main point). Via GetHostedZone we will be getting the list of zones we have access to, hence any issue raised by List/Change actions are not due to IAM limitations.

@sstarcher
Copy link
Contributor Author

@ideahitme sounds reasonable from an implementation standpoint.

@linki linki modified the milestone: v0.4 Jun 12, 2017
@linki linki added kind/bug Categorizes issue or PR as related to a bug. size/large labels Jun 12, 2017
@linki linki modified the milestones: v0.5, v0.4 Jun 23, 2017
@linki
Copy link
Member

linki commented Jun 23, 2017

moving this out of v0.4

@redbaron
Copy link
Contributor

redbaron commented Aug 2, 2017

What about Tags discovery? Then we specify Tag to filter hosted zones by

@ideahitme
Copy link

@redbaron I think it is definitely a great idea, which we should try out in favour of IAM regulated filtering

@linki linki added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/large labels Jan 2, 2018
lou-lan pushed a commit to lou-lan/external-dns that referenced this issue May 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants