rgw: S3 Bucket Policy Conditions IpAddress and NotIpAddress do not work #17010
Conversation
| m.addr |= Address(a.sin6_addr.s6_addr[13]) << 104; | ||
| m.addr |= Address(a.sin6_addr.s6_addr[14]) << 112; | ||
| m.addr |= Address(a.sin6_addr.s6_addr[15]) << 120; | ||
| m.addr |= Address(a.s6_addr[15]) << 0; |
|
(this was supposed to be a comment) |
|
This PR is targeting luminous - is luminous still getting merged to master? |
|
I believe this PR should be retargeted to master, and then cherry-pick to luminous after merge. Correct me if I'm wrong! |
|
@jgibson can you rebase this PR against ceph master and retarget the PR against master... The process is that PRs should target master and then we backport them back to Luminous |
|
make check failure: |
|
Sure, I can rebase against master, @theanalyst. I targeted luminous because I was fixing a bug in an existing release and didn't read the instructions in SubmittingPatches.rst closely enough. Although first I'll try to figure out why the new IP address unit test failed (they were fine on my dev box). What build environment do you use? |
jgibson
force-pushed
the
bugfix-rgw-s3-policy-ip-address-condition
branch
from
54ee397
to
fd66761
Compare
jgibson
force-pushed
the
bugfix-rgw-s3-policy-ip-address-condition
branch
3 times, most recently
from
bfeb0d5
to
305c64f
Compare
|
OK, I finally sorted out my mistake in the earlier pushes. It took longer than I would've liked, but on the upside the tests are more comprehensive now. Let me know if there's anything more that you need me to do to get this pull request merged. |
jgibson
force-pushed
the
bugfix-rgw-s3-policy-ip-address-condition
branch
from
305c64f
to
2c31557
Compare
jgibson
force-pushed
the
bugfix-rgw-s3-policy-ip-address-condition
branch
2 times, most recently
from
a830360
to
ea96ae7
Compare
|
Further testing revealed that the bucket policies were being evaluated against the wrong http headers. I've added changes that correct the issue. This includes respecting the Note that for the luminous backport you will also have to cherry pick 650d30d to have civetweb expose the remote ip address to rgw. |
|
Is this still relevant? I think we had another PR patching this that got merged a bit ago. |
|
I can't find the relevant PR, so I'm not sure. We've needed the IP address conditions for production and have just been running from a version of luminous compiled with those changes for a while now. I do see that now there are some conflicting files. I'll see if I can see what changed. |
|
OK, I took a look at master and this is definitely still relevant. At a minimum the current version is missing support for a custom remote address header, which is necessary for hosting ceph behind a reverse proxy and having the IP address conditions still be useful. I'd also wager that master's NotIpAddress condition will not work properly when there are multiple addresses or ranges in the list. In addition I didn't see any unit tests of the new functionality on master, whereas this PR has several. I can take a look at updating this PR to merge cleanly with the changes that have already been made soon. |
|
All right, thank you. As soon as we can get this clean I'll try and get it merged in. |
jgibson
force-pushed
the
bugfix-rgw-s3-policy-ip-address-condition
branch
from
ea96ae7
to
0ee2253
Compare
Signed-off-by: John Gibson <jgibson@mitre.org>
Signed-off-by: John Gibson <jgibson@mitre.org>
Comparisons of two individual IP addresses caused an assertion error. The text conversion of IPv4 addresses was using raw byte values instead of the converted number. NotIpAddress condition now works with multiple values. http://tracker.ceph.com/issues/20991 Signed-off-by: John Gibson <jgibson@mitre.org>
The IPv6 conversion was not properly converting the address to host byte order. The text conversion of IPv6 addresses was using raw byte values instead of the converted number. The portions of the addresses were grouped by bytes instead of 16-bit words. The prefix length was erroneously being rendered in hex. http://tracker.ceph.com/issues/20991 Signed-off-by: John Gibson <jgibson@mitre.org>
Signed-off-by: John Gibson <jgibson@mitre.org>
Previously bucket policy ip address restrictions were only being evaluated against the REMOTE_ADDR environment variable and ignoring the header specified by the rgw_remote_addr_param configuration option. This rendered ip-based bucket policies worthless when running behind a reverse proxy. Signed-off-by: John Gibson <jgibson@mitre.org>
Signed-off-by: John Gibson <jgibson@mitre.org>
Signed-off-by: John Gibson <jgibson@mitre.org>
jgibson
force-pushed
the
bugfix-rgw-s3-policy-ip-address-condition
branch
from
0ee2253
to
5f7d9c4
Compare
|
OK, I've resolved the conflicts. The changes introduced in 2fb445b (and possibly b3118ca) did resolve most of the problems with IPv4 addresses. The remaining changes in this PR fix IPv6, the string representation of the MaskedIPs, and support for NotIpAddress when using multiple values in the condition. In addition it adds support for evaluating against the request header specified rgw_remote_addr_param (typically used by reverse proxies). It also includes proper support for the X-Forwarded-For header when a request has passed through multiple remote proxies. Note that I've only verified this last round of changes via the unit tests. A full test on our running system (running luminous under Rook) will have to wait until after New Year's. |
|
@yuriw Could you include this in your next QA run? |
|
@yuriw Never mind! My QA run actually finished. Sorry for the false alarm. |
This fixes bugs in rgw's support for the bucket policy conditions IpAddress and NotIpAddress.
The only concern I have with the patch is getting the correct byte order for the ip addresses themselves on big endian architectures. (Although I don't know if ceph actually suppports big endian architectures.)
http://tracker.ceph.com/issues/20991