Repository navigation
tentacle: rgw: match CORS rules like Amazon S3 - #69647
Conversation
Select the first CORSRule whose AllowedOrigin, AllowedMethod, and (for preflight) AllowedHeader all match the request. Previously host_name_rule matched origin only, so a second rule with the same origin but different methods was never used. Add RGWCORSRule::matches() and RGWCORSConfiguration::match_rule(), wire generate_cors_headers and RGWOptionsCORS to them (including global CORS), and add unittest_rgw_cors. Signed-off-by: ramin.najarbashi <ramin.najarbashi@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit cc1dcec) Conflicts: src/rgw/rgw_op.cc Dropped optional_global_cors and validate_global_cors_request() because tentacle does not have global CORS. Applied match_rule() for bucket CORS only, with req_meth/req_hdrs captured in the lambda (as in main PR ceph#69190 review feedback).
Signed-off-by: ramin.najarbashi <ramin.najarbashi@gmail.com> (cherry picked from commit bbec695)
Signed-off-by: ramin.najarbashi <ramin.najarbashi@gmail.com> (cherry picked from commit b5624b6)
- matches_method: reject null/empty method; short-circuit RGW_CORS_ALL - matches_preflight_headers: add logging for skip and disallowed headers - generate_cors_headers: capture req_meth/req_hdrs in lambda - remove unused validate_cors_rule_method/header helpers Signed-off-by: ramin.najarbashi <ramin.najarbashi@gmail.com> On branch fix/rgw-cors-aws-rule-matching Changes to be committed: modified: src/rgw/rgw_cors.cc modified: src/rgw/rgw_op.cc (cherry picked from commit 8b3f907)
get_multi_cors_method_flags() calls ceph::for_each_substr(), but rgw_cors.h did not include str_list.h. The CORS unit test includes only this header, so the build failed with an undeclared identifier. Qualify the call and add the missing include. Signed-off-by: ramin.najarbashi <ramin.najarbashi@gmail.com> (cherry picked from commit 830f0e5)
unittest_rgw_cors used GMock::Main without global_init, so dout() in RGWCORSRule::matches() dereferenced a null g_ceph_context and segfaulted on the first test. Link unit-main like unittest_rgw_compression. Assisted-by: Cursor:composer-2.5-fast Signed-off-by: ramin.najarbashi <ramin.najarbashi@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> (cherry picked from commit c147865)
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
14 similar comments
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
Thank you for your contribution. Since you, the author, are not a member of the Ceph GitHub Org yet, our CI will not automatically run. Any member of the Ceph Org may comment "ok - to - test" (without the dashes) to allow the Jenkins jobs to run. |
|
updated pr description with
|
batrick
added
the
releng-audit-fail
github-actions
Bot
removed
the
releng-audit-fail
github-actions
Bot
removed
the
releng-audit-fail
batrick
added
the
releng-audit-fail
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
github-actions
Bot
added
releng-audit-fail
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Hi Backport Audit flagged a deviation (I think it comes from how to resolve cherry-pick conflicts in rgw_op. cc. I removed the global CORS sections because the fork doesn't have them). Can someone from @ceph/rgw have a look at it? If it looks good, /audit override helps. I also need ok to test Jenkins. I can't enable it myself. 🙂 |
batrick
added
the
releng-audit-queue
github-actions
Bot
removed
releng-audit-queue
There was a problem hiding this comment.
Ceph Release Engineering Audit Report
Commit Parity Visualizer
| BACKPORT PR #69647 | SOURCE PR |
SOURCE STATUS |
|---|---|---|
| 5f25f2b rgw: match CORS rules like Amazon S3 | PR #69190 | cc1dcec rgw: match CORS rules like Amazon S3 |
| d0786f5 reject null or empty method in matches_method | bbec695 reject null or empty method in matches_method | |
| ac2488b fix: some issue based on CORS spec 6.2.4 | b5624b6 fix: some issue based on CORS spec 6.2.4 | |
| 300d86e rgw: address CORS rule-matching review feedback | 8b3f907 rgw: address CORS rule-matching review feedback | |
| 2df5713 rgw: include str_list.h for ceph::for_each_substr in CORS header | 830f0e5 rgw: include str_list.h for ceph::for_each_substr in CORS header | |
| b0b5243 test/rgw: init CephContext in unittest_rgw_cors | c147865 test/rgw: init CephContext in unittest_rgw_cors |
Automated Backport Parity Review - Cherry-Pick Conflicts / Deviations
A conflict or deviation was detected during the simulation of this backport. The code in this PR does not match a clean cherry-pick of the upstream commits.
This does not necessarily indicate an issue but a maintainer should review.
**Click to expand conflict summaries.**
Deviation in Backport 5f25f2b (cherry-pick of cc1dcec)
Affected File(s)
src/rgw/rgw_op.cc
Range Diff
Click to expand
--- Original (cc1dcece)
+++ Backport (5f25f2bb)
@@ -11,3 +11,11 @@
Signed-off-by: ramin.najarbashi <ramin.najarbashi@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
+(cherry picked from commit cc1dcecee0892d7552ead76e3fb534e95df15fc5)
+
+Conflicts:
+ src/rgw/rgw_op.cc
+ Dropped optional_global_cors and validate_global_cors_request() because
+ tentacle does not have global CORS. Applied match_rule() for bucket CORS
+ only, with req_meth/req_hdrs captured in the lambda (as in main PR #69190
+ review feedback).
================================================================================
RANGE DIFF
================================================================================
1: cc1dcecee08 ! 1: 5f25f2bb0e2 rgw: match CORS rules like Amazon S3
@@ Commit message
Signed-off-by: ramin.najarbashi <ramin.najarbashi@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
+ (cherry picked from commit cc1dcecee0892d7552ead76e3fb534e95df15fc5)
+
+ Conflicts:
+ src/rgw/rgw_op.cc
+ Dropped optional_global_cors and validate_global_cors_request() because
+ tentacle does not have global CORS. Applied match_rule() for bucket CORS
+ only, with req_meth/req_hdrs captured in the lambda (as in main PR #69190
+ review feedback).
## src/rgw/rgw_cors.cc ##
@@
@@ src/rgw/rgw_cors.cc: void RGWCORSRule::format_exp_headers(string& s) {
## src/rgw/rgw_cors.h ##
@@ src/rgw/rgw_cors.h: public:
- void dump_origins();
+ void dump_origins();
void dump(Formatter *f) const;
bool is_header_allowed(const char *hdr, size_t len);
+ bool matches_method(const char *req_meth);
@@ src/rgw/rgw_op.cc: bool RGWOp::generate_cors_headers(string& origin, string& met
- /* CORS 6.2.2. */
- RGWCORSRule *rule = bucket_cors.host_name_rule(orig);
+- if (!rule)
+- return false;
+-
+- /*
+- * Set the Allowed-Origin header to a asterisk if this is allowed in the rule
+- * and no Authorization was send by the client
+- *
+- * The origin parameter specifies a URI that may access the resource. The browser must enforce this.
+- * For requests without credentials, the server may specify "*" as a wildcard,
+- * thereby allowing any origin to access the resource.
+- */
+- const char *authorization = s->info.env->get("HTTP_AUTHORIZATION");
+- if (!authorization && rule->has_wildcard_origin())
+- origin = "*";
+-
+- /* CORS 6.2.3. */
+ /* CORS 6.2.2–6.2.5: first rule matching origin, method, and preflight headers. */
-+ const char *req_meth = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_METHOD");
-+ if (!req_meth) {
-+ req_meth = s->info.method;
-+ }
+ const char *req_meth = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_METHOD");
+ if (!req_meth) {
+ req_meth = s->info.method;
+ }
+ const char *req_hdrs = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_HEADERS");
+
+ RGWCORSRule *rule = bucket_cors.match_rule(orig, req_meth, req_hdrs);
- auto is_allowed_to_generate_rule_cors_header = [this, &origin, &method, &headers, &exp_headers, &max_age] (RGWCORSRule *rule) {
- if (!rule)
- return false;
-@@ src/rgw/rgw_op.cc: bool RGWOp::generate_cors_headers(string& origin, string& method, string& header
- origin = "*";
-
- /* CORS 6.2.3. */
-- const char *req_meth = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_METHOD");
-- if (!req_meth) {
-- req_meth = s->info.method;
-+ const char *req_meth_inner = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_METHOD");
-+ if (!req_meth_inner) {
-+ req_meth_inner = s->info.method;
- }
++ auto is_allowed_to_generate_rule_cors_header = [this, &origin, &method, &headers, &exp_headers, &max_age, req_meth, req_hdrs] (RGWCORSRule *rule) {
++ if (!rule)
++ return false;
++
++ /*
++ * Set the Allowed-Origin header to a asterisk if this is allowed in the rule
++ * and no Authorization was send by the client
++ *
++ * The origin parameter specifies a URI that may access the resource. The browser must enforce this.
++ * For requests without credentials, the server may specify "*" as a wildcard,
++ * thereby allowing any origin to access the resource.
++ */
++ const char *authorization = s->info.env->get("HTTP_AUTHORIZATION");
++ if (!authorization && rule->has_wildcard_origin())
++ origin = "*";
-- if (req_meth) {
-- method = req_meth;
-- /* CORS 6.2.5. */
-- if (!validate_cors_rule_method(this, rule, req_meth)) {
-- return false;
-- }
-+ if (req_meth_inner) {
-+ method = req_meth_inner;
+- if (req_meth) {
+- method = req_meth;
+- /* CORS 6.2.5. */
+- if (!validate_cors_rule_method(this, rule, req_meth)) {
+- return false;
++ /* CORS 6.2.3. */
++ if (req_meth) {
++ method = req_meth;
}
+- }
- /* CORS 6.2.4. */
-- const char *req_hdrs = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_HEADERS");
-+ const char *req_hdrs_inner = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_HEADERS");
-
- /* CORS 6.2.6. */
-- get_cors_response_headers(this, rule, req_hdrs, headers, exp_headers, max_age);
-+ get_cors_response_headers(this, rule, req_hdrs_inner, headers, exp_headers, max_age);
+- /* CORS 6.2.4. */
+- const char *req_hdrs = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_HEADERS");
++ /* CORS 6.2.6. */
++ get_cors_response_headers(this, rule, req_hdrs, headers, exp_headers, max_age);
- return true;
- };
-@@ src/rgw/rgw_op.cc: bool RGWOp::generate_cors_headers(string& origin, string& method, string& header
- return true;
- }
+- /* CORS 6.2.6. */
+- get_cors_response_headers(this, rule, req_hdrs, headers, exp_headers, max_age);
++ return true;
++ };
-- if (optional_global_cors.has_value() && is_allowed_to_generate_rule_cors_header(&(*optional_global_cors))) {
-+ if (optional_global_cors.has_value() &&
-+ optional_global_cors->matches(orig, req_meth, req_hdrs) &&
-+ is_allowed_to_generate_rule_cors_header(&(*optional_global_cors))) {
- return true;
- }
+- return true;
++ if (is_allowed_to_generate_rule_cors_header(rule)) {
++ return true;
++ }
++
++ return false;
+ }
+ int rgw_policy_from_attrset(const DoutPrefixProvider *dpp, CephContext *cct, map<string, bufferlist>& attrset, RGWAccessControlPolicy *policy)
@@ src/rgw/rgw_op.cc: void RGWOptionsCORS::get_response_params(string& hdrs, string& exp_hdrs, unsigne
}
@@ src/rgw/rgw_op.cc: void RGWOptionsCORS::get_response_params(string& hdrs, string
-
- if (!validate_cors_rule_header(this, rule, req_hdrs)) {
+ ldpp_dout(this, 10) << "no CORS rule matching origin=" << origin
-+ << " method=" << req_meth << dendl;
- return -ENOENT;
- }
-
-@@ src/rgw/rgw_op.cc: int RGWOptionsCORS::validate_cors_request(RGWCORSConfiguration *cc) {
- int RGWOptionsCORS::validate_global_cors_request(RGWCORSRule *global_cors_rule) {
- ldpp_dout(this, 20) << "Validating request with global CORS" << dendl;
- rule = global_cors_rule;
-- if (!rule) {
-- ldpp_dout(this, 10) << "There is no global cors rule present" << dendl;
-- return -ENOENT;
-- }
--
-- if (!rule->is_origin_present(origin)) {
-- ldpp_dout(this, 10) << "There is no cors rule present for " << origin << dendl;
-- return -ENOENT;
-- }
--
-- if (!validate_cors_rule_method(this, rule, req_meth)) {
-- return -ENOENT;
-- }
--
-- if (!validate_cors_rule_header(this, rule, req_hdrs)) {
-+ if (!rule || !rule->matches(origin, req_meth, req_hdrs)) {
-+ ldpp_dout(this, 10) << "no global CORS rule matching origin=" << origin
+ << " method=" << req_meth << dendl;
return -ENOENT;
}Deviation in Backport 300d86e (cherry-pick of 8b3f907)
Affected File(s)
src/rgw/rgw_op.cc
Range Diff
Click to expand
--- Original (8b3f907f)
+++ Backport (300d86e9)
@@ -11,3 +11,5 @@
Changes to be committed:
modified: src/rgw/rgw_cors.cc
modified: src/rgw/rgw_op.cc
+
+(cherry picked from commit 8b3f907f7c819b16f098142c9242d2abb6fa2829)
================================================================================
RANGE DIFF
================================================================================
1: 8b3f907f7c8 ! 1: 300d86e9786 rgw: address CORS rule-matching review feedback
@@ Commit message
modified: src/rgw/rgw_cors.cc
modified: src/rgw/rgw_op.cc
+ (cherry picked from commit 8b3f907f7c819b16f098142c9242d2abb6fa2829)
+
## src/rgw/rgw_cors.cc ##
@@ src/rgw/rgw_cors.cc: bool RGWCORSRule::matches_preflight_headers(const char *req_hdrs)
get_str_vec(req_hdrs, hdrs);
@@ src/rgw/rgw_op.cc: int RGWOp::init_quota()
- return false;
- }
-
-- uint8_t flags = get_multi_cors_method_flags(req_meth);
+- uint8_t flags = get_cors_method_flags(req_meth);
-
- if (rule->get_allowed_methods() & flags) {
- ldpp_dout(dpp, 10) << "Method " << req_meth << " is supported" << dendl;
@@ src/rgw/rgw_op.cc: int RGWOp::init_quota()
int RGWOp::read_bucket_cors()
{
bufferlist bl;
-@@ src/rgw/rgw_op.cc: bool RGWOp::generate_cors_headers(string& origin, string& method, string& header
- const char *req_hdrs = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_HEADERS");
-
- RGWCORSRule *rule = bucket_cors.match_rule(orig, req_meth, req_hdrs);
-- auto is_allowed_to_generate_rule_cors_header = [this, &origin, &method, &headers, &exp_headers, &max_age] (RGWCORSRule *rule) {
-+ auto is_allowed_to_generate_rule_cors_header = [this, &origin, &method, &headers, &exp_headers, &max_age, req_meth, req_hdrs] (RGWCORSRule *rule) {
- if (!rule)
- return false;
-
-@@ src/rgw/rgw_op.cc: bool RGWOp::generate_cors_headers(string& origin, string& method, string& header
- origin = "*";
-
- /* CORS 6.2.3. */
-- const char *req_meth_inner = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_METHOD");
-- if (!req_meth_inner) {
-- req_meth_inner = s->info.method;
-+ if (req_meth) {
-+ method = req_meth;
- }
-
-- if (req_meth_inner) {
-- method = req_meth_inner;
-- }
--
-- /* CORS 6.2.4. */
-- const char *req_hdrs_inner = s->info.env->get("HTTP_ACCESS_CONTROL_REQUEST_HEADERS");
--
- /* CORS 6.2.6. */
-- get_cors_response_headers(this, rule, req_hdrs_inner, headers, exp_headers, max_age);
-+ get_cors_response_headers(this, rule, req_hdrs, headers, exp_headers, max_age);
-
- return true;
- };Deviation in Backport 2df5713 (cherry-pick of 830f0e5)
Affected File(s)
src/rgw/rgw_cors.h
Range Diff
Click to expand
--- Original (830f0e56)
+++ Backport (2df5713b)
@@ -6,3 +6,4 @@
Qualify the call and add the missing include.
Signed-off-by: ramin.najarbashi <ramin.najarbashi@gmail.com>
+(cherry picked from commit 830f0e562851a36abc2b450fcf0dd3e554abf24f)
================================================================================
RANGE DIFF
================================================================================
1: 830f0e56285 < -: ----------- rgw: include str_list.h for ceph::for_each_substr in CORS header
-: ----------- > 1: 2df5713bfbc rgw: include str_list.h for ceph::for_each_substr in CORS headerHow to proceed:
- Authors (Genuine Conflicts): If this is a genuine conflict requiring manual resolution, ensure your resolution is correct. You must explain the conflict resolution in the commit message (e.g., leave the standard Git
Conflicts:block intact) and include an explanation for changes. - Authors (Need Help?): Reach out to the Component Lead for technical guidance on complex code conflicts.
- Component Leads (Review): Please review the Range Diff(s) above to verify the author's manual conflict resolution is correct for this release branch. If the deviation is intentional, documented, and approved then the component lead or @ceph/ceph-release-manager can bypass this check by commenting
/audit override.
Be familiar with the rules and guidelines for writing backports.
🛟 Need Help?
If you need technical help resolving these issues, please consult with the Component Lead. If you need administrative overrides, please see the #ceph-upstream-releases channel on Slack and request a review from the @ceph/ceph-release-manager.
📋 Component Lead / Release Manager
To override the audit failure, apply releng-audit-override label or comment /audit override.
When you are ready for a new audit, please remove the releng-audit-fail label or comment /audit retest.
github-actions
Bot
added
the
releng-audit-fail
|
Hi @batrick — thanks for the help earlier. You kicked the audit again via the queue label, but it failed the same way. I think that's expected (intentional tentacle conflict on the CORS cherry-pick). Do I need to do anything else, or should we just get an |
|
/audit override |
github-actions
Bot
added
releng-audit-override
and removed
releng-audit-fail
|
✅ Audit Override Applied by @adamemerson. |
|
ok to test |
|
This PR has been added to tentacle integration testing by anuchaithra started 2026-07-29-08:36. |
pr testing completed and got approval from @ivancich. more data : https://tracker.ceph.com/issues/78821 |
6 participants
Summary
Backport of #69190 to tentacle.
RGW previously selected a CORS rule with
host_name_rule()(origin only), thenvalidated method and headers on that single rule. Bucket configs with multiple
CORSRuleelements sharing the sameAllowedOriginbut differentAllowedMethodvalues could fail preflight when a later rule should match.This aligns rule selection with Amazon S3 (first rule matching origin, method,
and preflight headers).
Why tentacle: the buggy origin-only matching exists on tentacle; users running
multi-rule bucket CORS on tentacle RGW hit the same preflight failures fixed on
main.
Minimal change: cherry-pick of all six commits from PR #69190 with
git cherry-pick -x.Conflict in
rgw_op.ccresolved by dropping global CORS(
optional_global_cors) because tentacle does not have global CORS; bucketCORS uses
match_rule()only.Parent tracker: https://tracker.ceph.com/issues/77575
Official backport tracker: https://tracker.ceph.com/issues/77584
Testing
unittest_rgw_cors(from main PR)Contribution Guidelines
Checklist
Fixes: https://tracker.ceph.com/issues/77584