public inbox for [email protected]help / color / mirror / Atom feed
[PATCH 4/5] WIP rework tracking of expressions 8+ messages / 4 participants [nested] [flat]
* [PATCH 4/5] WIP rework tracking of expressions @ 2021-02-17 00:02 Tomas Vondra <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Tomas Vondra @ 2021-02-17 00:02 UTC (permalink / raw) --- src/backend/statistics/dependencies.c | 136 ++++++++++++++---- src/backend/statistics/extended_stats.c | 73 ++++------ src/backend/statistics/mcv.c | 26 +++- src/backend/statistics/mvdistinct.c | 120 +++++++++------- src/backend/utils/adt/selfuncs.c | 29 +++- .../statistics/extended_stats_internal.h | 11 +- src/include/statistics/statistics.h | 3 +- 7 files changed, 255 insertions(+), 143 deletions(-) diff --git a/src/backend/statistics/dependencies.c b/src/backend/statistics/dependencies.c index 6bf3127bcc..602301b724 100644 --- a/src/backend/statistics/dependencies.c +++ b/src/backend/statistics/dependencies.c @@ -252,7 +252,7 @@ dependency_degree(int numrows, HeapTuple *rows, ExprInfo *exprs, int k, * member easier, and then construct a filtered version with only attnums * referenced by the dependency we validate. */ - attnums = build_attnums_array(attrs, &numattrs); + attnums = build_attnums_array(attrs, exprs->nexprs, &numattrs); attnums_dep = (AttrNumber *) palloc(k * sizeof(AttrNumber)); for (i = 0; i < k; i++) @@ -372,19 +372,46 @@ statext_dependencies_build(int numrows, HeapTuple *rows, k; int numattrs; AttrNumber *attnums; + int nattnums; /* result */ MVDependencies *dependencies = NULL; - /* treat expressions as special attributes with high attnums */ - attrs = add_expressions_to_attributes(attrs, exprs->nexprs); + /* + * Transform the bms into an array of attnums, to make accessing i-th + * member easier. We add the expressions first, represented by negative + * attnums (this is OK, we don't allow statistics on system attributes), + * and then regular attributes. + */ + nattnums = bms_num_members(attrs) + exprs->nexprs; + attnums = (AttrNumber *) palloc(sizeof(AttrNumber) * nattnums); + + numattrs = 0; + + /* treat expressions as attributes with negative attnums */ + for (i = 0; i < exprs->nexprs; i++) + attnums[numattrs++] = -(i+1); /* - * Transform the bms into an array, to make accessing i-th member easier. + * and then regular attributes + * + * XXX Maybe add this in the opposite order, just like in MCV? first + * regular attnums, then exressions. */ - attnums = build_attnums_array(attrs, &numattrs); + k = -1; + while ((k = bms_next_member(attrs, k)) >= 0) + attnums[numattrs++] = k; Assert(numattrs >= 2); + Assert(numattrs == nattnums); + + /* + * Build a new bitmapset of attnums, offset by number of expressions (this + * is needed, because bitmaps can store only non-negative values). + */ + attrs = NULL; + for (i = 0; i < numattrs; i++) + attrs = bms_add_member(attrs, attnums[i] + exprs->nexprs); /* * We'll try build functional dependencies starting from the smallest ones @@ -1374,8 +1401,10 @@ dependencies_clauselist_selectivity(PlannerInfo *root, * We also skip clauses that we already estimated using different types of * statistics (we treat them as incompatible). * - * For expressions, we generate attnums higher than MaxHeapAttributeNumber - * so that we can work with attnums only. + * To handle expressions, we assign them negative attnums, as if it was a + * system attribute (this is fine, as we only allow extended stats on user + * attributes). And then we offset everything by the number of expressions, + * so that we can store the values in a bitmapset. */ listidx = 0; foreach(l, clauses) @@ -1391,13 +1420,12 @@ dependencies_clauselist_selectivity(PlannerInfo *root, { /* * If it's a simple column refrence, just extract the attnum. If - * it's an expression, make sure it's not a duplicate and assign - * a special attnum to it (higher than any regular value). + * it's an expression, assign a negative attnum as if it was a + * system attribute. */ if (dependency_is_compatible_clause(clause, rel->relid, &attnum)) { list_attnums[listidx] = attnum; - clauses_attnums = bms_add_member(clauses_attnums, attnum); } else if (dependency_is_compatible_expression(clause, rel->relid, rel->statlist, @@ -1413,7 +1441,8 @@ dependencies_clauselist_selectivity(PlannerInfo *root, { if (equal(unique_exprs[i], expr)) { - attnum = EXPRESSION_ATTNUM(i); + /* negative attribute number to expression */ + attnum = -(i + 1); break; } } @@ -1421,14 +1450,10 @@ dependencies_clauselist_selectivity(PlannerInfo *root, /* not found in the list, so add it */ if (attnum == InvalidAttrNumber) { - attnum = EXPRESSION_ATTNUM(unique_exprs_cnt); unique_exprs[unique_exprs_cnt++] = expr; - /* shouldn't have seen this attnum yet */ - Assert(!bms_is_member(attnum, clauses_attnums)); - - /* we may add the attnum repeatedly to clauses_attnums */ - clauses_attnums = bms_add_member(clauses_attnums, attnum); + /* after incrementing the value, to get -1, -2, ... */ + attnum = -unique_exprs_cnt; } /* remember which attnum was assigned to this clause */ @@ -1439,6 +1464,37 @@ dependencies_clauselist_selectivity(PlannerInfo *root, listidx++; } + Assert(listidx == list_length(clauses)); + + /* + * Now that we know how many expressions there are, we can offset the + * values just enough to build the bitmapset. + */ + for (i = 0; i < list_length(clauses); i++) + { + AttrNumber attnum; + + /* ignore incompatible or already estimated clauses */ + if (list_attnums[i] == InvalidAttrNumber) + continue; + + /* make sure the attnum is in the expected range */ + Assert(list_attnums[i] >= (-unique_exprs_cnt)); + Assert(list_attnums[i] <= MaxHeapAttributeNumber); + + /* make sure the attnum is not negative */ + attnum = list_attnums[i] + unique_exprs_cnt; + + /* + * Expressions are unique, and so we must not have seen this attnum + * before. + */ + Assert(AttrNumberIsForUserDefinedAttr(list_attnums[i]) || + !bms_is_member(attnum, clauses_attnums)); + + clauses_attnums = bms_add_member(clauses_attnums, attnum); + } + /* * If there's not at least two distinct attnums and expressions, then * reject the whole list of clauses. We must return 1.0 so the calling @@ -1470,19 +1526,37 @@ dependencies_clauselist_selectivity(PlannerInfo *root, foreach(l, rel->statlist) { StatisticExtInfo *stat = (StatisticExtInfo *) lfirst(l); - Bitmapset *matched; int nmatched; int nexprs; + int k; MVDependencies *deps; /* skip statistics that are not of the correct type */ if (stat->kind != STATS_EXT_DEPENDENCIES) continue; - /* count matching simple clauses */ - matched = bms_intersect(clauses_attnums, stat->keys); - nmatched = bms_num_members(matched); - bms_free(matched); + /* + * Count matching attributes - we have to undo two attnum offsets. + * First, the dependency is offset using the number of expressions + * for that statistics, and then (if it's a plain attribute) we + * need to apply the same offset as above, by unique_exprs_cnt. + */ + nmatched = 0; + k = -1; + while ((k = bms_next_member(stat->keys, k)) >= 0) + { + AttrNumber attnum = (AttrNumber) k; + + /* skip expressions */ + if (!AttrNumberIsForUserDefinedAttr(attnum)) + continue; + + /* apply the same offset as above */ + attnum += unique_exprs_cnt; + + if (bms_is_member(attnum, clauses_attnums)) + nmatched++; + } /* count matching expressions */ nexprs = 0; @@ -1537,13 +1611,23 @@ dependencies_clauselist_selectivity(PlannerInfo *root, Node *expr; int k; AttrNumber unique_attnum = InvalidAttrNumber; + AttrNumber attnum; - /* regular attribute, no need to remap */ - if (dep->attributes[j] <= MaxHeapAttributeNumber) + /* undo the per-statistics offset */ + attnum = dep->attributes[j]; + + /* regular attribute, simply offset by number of expressions */ + if (AttrNumberIsForUserDefinedAttr(attnum)) + { + dep->attributes[j] = attnum + unique_exprs_cnt; continue; + } + + /* the attnum should be a valid system attnum (-1, -2, ...) */ + Assert(AttributeNumberIsValid(attnum)); /* index of the expression */ - idx = EXPRESSION_INDEX(dep->attributes[j]); + idx = (1 - attnum); /* make sure the expression index is valid */ Assert((idx >= 0) && (idx < list_length(stat->exprs))); @@ -1559,7 +1643,7 @@ dependencies_clauselist_selectivity(PlannerInfo *root, */ if (equal(unique_exprs[k], expr)) { - unique_attnum = EXPRESSION_ATTNUM(k); + unique_attnum = -(k + 1) + unique_exprs_cnt; break; } } diff --git a/src/backend/statistics/extended_stats.c b/src/backend/statistics/extended_stats.c index 6ed938d6ab..95b2cc683e 100644 --- a/src/backend/statistics/extended_stats.c +++ b/src/backend/statistics/extended_stats.c @@ -892,7 +892,7 @@ bsearch_arg(const void *key, const void *base, size_t nmemb, size_t size, * is not necessary here (and when querying the bitmap). */ AttrNumber * -build_attnums_array(Bitmapset *attrs, int *numattrs) +build_attnums_array(Bitmapset *attrs, int nexprs, int *numattrs) { int i, j; @@ -908,16 +908,19 @@ build_attnums_array(Bitmapset *attrs, int *numattrs) j = -1; while ((j = bms_next_member(attrs, j)) >= 0) { + AttrNumber attnum = (j - nexprs); + /* * Make sure the bitmap contains only user-defined attributes. As * bitmaps can't contain negative values, this can be violated in two * ways. Firstly, the bitmap might contain 0 as a member, and secondly * the integer value might be larger than MaxAttrNumber. */ - Assert(AttrNumberIsForUserDefinedAttr(j)); - Assert(j <= MaxAttrNumber); + Assert(AttributeNumberIsValid(attnum)); + Assert(attnum <= MaxAttrNumber); + Assert(attnum >= (-nexprs)); - attnums[i++] = (AttrNumber) j; + attnums[i++] = (AttrNumber) attnum; /* protect against overflows */ Assert(i <= num); @@ -984,15 +987,16 @@ build_sorted_items(int numrows, int *nitems, HeapTuple *rows, ExprInfo *exprs, Datum value; bool isnull; int attlen; + AttrNumber attnum = attnums[j]; - if (attnums[j] <= MaxHeapAttributeNumber) + if (AttrNumberIsForUserDefinedAttr(attnum)) { - value = heap_getattr(rows[i], attnums[j], tdesc, &isnull); - attlen = TupleDescAttr(tdesc, attnums[j] - 1)->attlen; + value = heap_getattr(rows[i], attnum, tdesc, &isnull); + attlen = TupleDescAttr(tdesc, attnum - 1)->attlen; } else { - int idx = EXPRESSION_INDEX(attnums[j]); + int idx = -(attnums[j] + 1); Assert((idx >= 0) && (idx < exprs->nexprs)); @@ -1097,6 +1101,21 @@ stat_find_expression(StatisticExtInfo *stat, Node *expr) return -1; } +static bool +stat_covers_attributes(StatisticExtInfo *stat, Bitmapset *attnums) +{ + int k; + + k = -1; + while ((k = bms_next_member(attnums, k)) >= 0) + { + if (!bms_is_member(k, stat->keys)) + return false; + } + + return true; +} + /* * stat_covers_expressions * Test whether a statistics object covers all expressions in a list. @@ -1181,7 +1200,7 @@ choose_best_statistics(List *stats, char requiredkind, continue; /* ignore clauses that are not covered by this object */ - if (!bms_is_subset(clause_attnums[i], info->keys) || + if (!stat_covers_attributes(info, clause_attnums[i]) || !stat_covers_expressions(info, clause_exprs[i], &expr_idxs)) continue; @@ -1685,7 +1704,7 @@ statext_mcv_clauselist_selectivity(PlannerInfo *root, List *clauses, int varReli * estimate. */ if (!bms_is_member(listidx, *estimatedclauses) && - bms_is_subset(list_attnums[listidx], stat->keys) && + stat_covers_attributes(stat, list_attnums[listidx]) && stat_covers_expressions(stat, list_exprs[listidx], NULL)) { /* record simple clauses (single column or expression) */ @@ -2555,37 +2574,3 @@ evaluate_expressions(Relation rel, List *exprs, int numrows, HeapTuple *rows) return result; } - -/* - * add_expressions_to_attributes - * add expressions as attributes with high attnums - * - * Treat the expressions as attributes with attnums above the regular - * attnum range. This will allow us to handle everything in the same - * way, and identify expressions in the dependencies. - * - * XXX This always creates a copy of the bitmap. We might optimize this - * by only creating the copy with (nexprs > 0) but then we'd have to track - * this in order to free it (if we want to). Does not seem worth it. - */ -Bitmapset * -add_expressions_to_attributes(Bitmapset *attrs, int nexprs) -{ - int i; - - /* - * Copy the bitmapset and add fake attnums representing expressions, - * starting above MaxHeapAttributeNumber. - */ - attrs = bms_copy(attrs); - - /* start with (MaxHeapAttributeNumber + 1) */ - for (i = 0; i < nexprs; i++) - { - Assert(EXPRESSION_ATTNUM(i) > MaxHeapAttributeNumber); - - attrs = bms_add_member(attrs, EXPRESSION_ATTNUM(i)); - } - - return attrs; -} diff --git a/src/backend/statistics/mcv.c b/src/backend/statistics/mcv.c index 3bb6fa733d..323d476814 100644 --- a/src/backend/statistics/mcv.c +++ b/src/backend/statistics/mcv.c @@ -187,6 +187,7 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, double totalrows, int stattarget) { int i, + k, numattrs, ngroups, nitems; @@ -206,10 +207,26 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, * XXX We do this after build_mss, because that expects the bitmapset * to only contain simple attributes (with a matching VacAttrStats) */ - attrs = add_expressions_to_attributes(attrs, exprs->nexprs); - /* now build the array, with the special expression attnums */ - attnums = build_attnums_array(attrs, &numattrs); + /* + * Transform the bms into an array, to make accessing i-th member easier. + */ + attnums = (AttrNumber *) palloc(sizeof(AttrNumber) * (bms_num_members(attrs) + exprs->nexprs)); + + numattrs = 0; + + /* regular attributes */ + k = -1; + while ((k = bms_next_member(attrs, k)) >= 0) + attnums[numattrs++] = k; + + /* treat expressions as attributes with negative attnums */ + for (i = 0; i < exprs->nexprs; i++) + attnums[numattrs++] = -(i+1); + + Assert(numattrs >= 2); + Assert(numattrs == (bms_num_members(attrs) + exprs->nexprs)); + /* sort the rows */ items = build_sorted_items(numrows, &nitems, rows, exprs, @@ -349,7 +366,6 @@ statext_mcv_build(int numrows, HeapTuple *rows, ExprInfo *exprs, pfree(items); pfree(groups); - pfree(attrs); return mcvlist; } @@ -1692,6 +1708,8 @@ mcv_get_match_bitmap(PlannerInfo *root, List *clauses, bool match = true; MCVItem *item = &mcvlist->items[i]; + Assert(idx >= 0); + /* * When the MCV item or the Const value is NULL we can * treat this as a mismatch. We must not call the operator diff --git a/src/backend/statistics/mvdistinct.c b/src/backend/statistics/mvdistinct.c index 55d3fa0e1f..5e796e7123 100644 --- a/src/backend/statistics/mvdistinct.c +++ b/src/backend/statistics/mvdistinct.c @@ -83,9 +83,9 @@ static void generate_combinations(CombinationGenerator *state); * This computes the ndistinct estimate using the same estimator used * in analyze.c and then computes the coefficient. * - * To handle expressions easily, we treat them as special attributes with - * attnums above MaxHeapAttributeNumber, and we assume the expressions are - * placed after all simple attributes. + * To handle expressions easily, we treat them as system attributes with + * negative attnums, and offset everything by number of expressions to + * allow using Bitmapsets. */ MVNDistinct * statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows, @@ -93,10 +93,12 @@ statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows, VacAttrStats **stats) { MVNDistinct *result; + int i; int k; int itemcnt; int numattrs = bms_num_members(attrs); int numcombs = num_combinations(numattrs + exprs->nexprs); + Bitmapset *tmp = NULL; result = palloc(offsetof(MVNDistinct, items) + numcombs * sizeof(MVNDistinctItem)); @@ -104,8 +106,26 @@ statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows, result->type = STATS_NDISTINCT_TYPE_BASIC; result->nitems = numcombs; - /* treat expressions as special attributes with high attnums */ - attrs = add_expressions_to_attributes(attrs, exprs->nexprs); + /* + * Treat expressions as system attributes with negative attnums, + * but offset everything by number of expressions. + */ + for (i = 0; i < exprs->nexprs; i++) + { + AttrNumber attnum = -(i + 1); + tmp = bms_add_member(tmp, attnum + exprs->nexprs); + } + + /* regular attributes */ + k = -1; + while ((k = bms_next_member(attrs, k)) >= 0) + { + AttrNumber attnum = k; + tmp = bms_add_member(tmp, attnum + exprs->nexprs); + } + + /* use the newly built bitmapset */ + attrs = tmp; /* make sure there were no clashes */ Assert(bms_num_members(attrs) == numattrs + exprs->nexprs); @@ -124,29 +144,33 @@ statext_ndistinct_build(double totalrows, int numrows, HeapTuple *rows, MVNDistinctItem *item = &result->items[itemcnt]; int j; - item->attrs = NULL; + item->attributes = palloc(sizeof(AttrNumber) * k); + item->nattributes = k; + for (j = 0; j < k; j++) { AttrNumber attnum = InvalidAttrNumber; /* - * The simple attributes are before expressions, so have - * indexes below numattrs. - * */ - if (combination[j] < numattrs) - attnum = stats[combination[j]]->attr->attnum; + * The expressions have negative attnums, so even with the + * offset are before regular attributes. So the first chunk + * of indexes are for expressions. + */ + if (combination[j] >= exprs->nexprs) + attnum + = stats[combination[j] - exprs->nexprs]->attr->attnum; else { /* make sure the expression index is valid */ - Assert((combination[j] - numattrs) >= 0); - Assert((combination[j] - numattrs) < exprs->nexprs); + Assert(combination[j] >= 0); + Assert(combination[j] < exprs->nexprs); - attnum = EXPRESSION_ATTNUM(combination[j] - numattrs); + attnum = -(combination[j] + 1); } Assert(attnum != InvalidAttrNumber); - item->attrs = bms_add_member(item->attrs, attnum); + item->attributes[j] = attnum; } item->ndistinct = @@ -223,7 +247,7 @@ statext_ndistinct_serialize(MVNDistinct *ndistinct) { int nmembers; - nmembers = bms_num_members(ndistinct->items[i].attrs); + nmembers = ndistinct->items[i].nattributes; Assert(nmembers >= 2); len += SizeOfItem(nmembers); @@ -248,22 +272,15 @@ statext_ndistinct_serialize(MVNDistinct *ndistinct) for (i = 0; i < ndistinct->nitems; i++) { MVNDistinctItem item = ndistinct->items[i]; - int nmembers = bms_num_members(item.attrs); - int x; + int nmembers = item.nattributes; memcpy(tmp, &item.ndistinct, sizeof(double)); tmp += sizeof(double); memcpy(tmp, &nmembers, sizeof(int)); tmp += sizeof(int); - x = -1; - while ((x = bms_next_member(item.attrs, x)) >= 0) - { - AttrNumber value = (AttrNumber) x; - - memcpy(tmp, &value, sizeof(AttrNumber)); - tmp += sizeof(AttrNumber); - } + memcpy(tmp, item.attributes, sizeof(AttrNumber) * nmembers); + tmp += nmembers * sizeof(AttrNumber); /* protect against overflows */ Assert(tmp <= ((char *) output + len)); @@ -335,27 +352,21 @@ statext_ndistinct_deserialize(bytea *data) for (i = 0; i < ndistinct->nitems; i++) { MVNDistinctItem *item = &ndistinct->items[i]; - int nelems; - - item->attrs = NULL; /* ndistinct value */ memcpy(&item->ndistinct, tmp, sizeof(double)); tmp += sizeof(double); /* number of attributes */ - memcpy(&nelems, tmp, sizeof(int)); + memcpy(&item->nattributes, tmp, sizeof(int)); tmp += sizeof(int); - Assert((nelems >= 2) && (nelems <= STATS_MAX_DIMENSIONS)); + Assert((item->nattributes >= 2) && (item->nattributes <= STATS_MAX_DIMENSIONS)); - while (nelems-- > 0) - { - AttrNumber attno; + item->attributes + = (AttrNumber *) palloc(item->nattributes * sizeof(AttrNumber)); - memcpy(&attno, tmp, sizeof(AttrNumber)); - tmp += sizeof(AttrNumber); - item->attrs = bms_add_member(item->attrs, attno); - } + memcpy(item->attributes, tmp, sizeof(AttrNumber) * item->nattributes); + tmp += sizeof(AttrNumber) * item->nattributes; /* still within the bytea */ Assert(tmp <= ((char *) data + VARSIZE_ANY(data))); @@ -403,17 +414,16 @@ pg_ndistinct_out(PG_FUNCTION_ARGS) for (i = 0; i < ndist->nitems; i++) { - MVNDistinctItem item = ndist->items[i]; - int x = -1; - bool first = true; + int j; + MVNDistinctItem item = ndist->items[i]; if (i > 0) appendStringInfoString(&str, ", "); - while ((x = bms_next_member(item.attrs, x)) >= 0) + for (j = 0; j < item.nattributes; j++) { - appendStringInfo(&str, "%s%d", first ? "\"" : ", ", x); - first = false; + AttrNumber attnum = item.attributes[j]; + appendStringInfo(&str, "%s%d", (j == 0) ? "\"" : ", ", attnum); } appendStringInfo(&str, "\": %d", (int) item.ndistinct); } @@ -508,9 +518,10 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows, TupleDesc tdesc = NULL; Oid collid = InvalidOid; - if (combination[i] < nattrs) + /* first nexprs indexes are for expressions, then regular attributes */ + if (combination[i] >= exprs->nexprs) { - VacAttrStats *colstat = stats[combination[i]]; + VacAttrStats *colstat = stats[combination[i] - exprs->nexprs]; typid = colstat->attrtypid; attnum = colstat->attr->attnum; collid = colstat->attrcollid; @@ -518,8 +529,8 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows, } else { - typid = exprs->types[combination[i] - nattrs]; - collid = exprs->collations[combination[i] - nattrs]; + typid = exprs->types[combination[i]]; + collid = exprs->collations[combination[i]]; } type = lookup_type_cache(typid, TYPECACHE_LT_OPR); @@ -534,10 +545,13 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows, for (j = 0; j < numrows; j++) { /* - * The first nattrs indexes identify simple attributes, higher - * indexes are expressions. + * The first exprs indexes identify expressions, higher indexes + * are for plain attributes. + * + * XXX This seems a bit strange that we don't offset the (i) + * in any way? */ - if (combination[i] < nattrs) + if (combination[i] >= exprs->nexprs) items[j].values[i] = heap_getattr(rows[j], attnum, @@ -545,7 +559,9 @@ ndistinct_for_combination(double totalrows, int numrows, HeapTuple *rows, &items[j].isnull[i]); else { - int idx = (combination[i] - nattrs); + /* we know the first nexprs expressions are expressions, + * and the value is directly the expression index */ + int idx = combination[i]; /* make sure the expression index is valid */ Assert((idx >= 0) && (idx < exprs->nexprs)); diff --git a/src/backend/utils/adt/selfuncs.c b/src/backend/utils/adt/selfuncs.c index a7edcaeaff..26e1912940 100644 --- a/src/backend/utils/adt/selfuncs.c +++ b/src/backend/utils/adt/selfuncs.c @@ -4144,7 +4144,8 @@ estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel, if (equal(exprinfo->expr, expr)) { - matched = bms_add_member(matched, MaxHeapAttributeNumber + idx); + AttrNumber attnum = -(idx + 1); + matched = bms_add_member(matched, attnum + list_length(matched_info->exprs)); found = true; break; } @@ -4165,10 +4166,10 @@ estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel, if (!AttrNumberIsForUserDefinedAttr(attnum)) continue; - if (!bms_is_member(attnum, matched_info->keys)) + if (!bms_is_member(attnum + list_length(matched_info->exprs), matched_info->keys)) continue; - matched = bms_add_member(matched, attnum); + matched = bms_add_member(matched, attnum + list_length(matched_info->exprs)); } } } @@ -4176,13 +4177,29 @@ estimate_multivariate_ndistinct(PlannerInfo *root, RelOptInfo *rel, /* Find the specific item that exactly matches the combination */ for (i = 0; i < stats->nitems; i++) { + int j; MVNDistinctItem *tmpitem = &stats->items[i]; - if (bms_subset_compare(tmpitem->attrs, matched) == BMS_EQUAL) + if (tmpitem->nattributes != bms_num_members(matched)) + continue; + + /* assume it's the right item */ + item = tmpitem; + + for (j = 0; j < tmpitem->nattributes; j++) { - item = tmpitem; - break; + AttrNumber attnum = tmpitem->attributes[j]; + + if (!bms_is_member(attnum, matched)) + { + /* nah, it's not this item */ + item = NULL; + break; + } } + + if (item) + break; } /* make sure we found an item */ diff --git a/src/include/statistics/extended_stats_internal.h b/src/include/statistics/extended_stats_internal.h index b2e59f9bc5..1f09799deb 100644 --- a/src/include/statistics/extended_stats_internal.h +++ b/src/include/statistics/extended_stats_internal.h @@ -106,7 +106,7 @@ extern void *bsearch_arg(const void *key, const void *base, int (*compar) (const void *, const void *, void *), void *arg); -extern AttrNumber *build_attnums_array(Bitmapset *attrs, int *numattrs); +extern AttrNumber *build_attnums_array(Bitmapset *attrs, int nexprs, int *numattrs); extern SortItem *build_sorted_items(int numrows, int *nitems, HeapTuple *rows, ExprInfo *exprs, TupleDesc tdesc, @@ -141,13 +141,4 @@ extern Selectivity mcv_clause_selectivity_or(PlannerInfo *root, Selectivity *overlap_basesel, Selectivity *totalsel); -extern Bitmapset *add_expressions_to_attributes(Bitmapset *attrs, int nexprs); - -/* translate 0-based expression index to attnum and back */ -#define EXPRESSION_ATTNUM(index) \ - (MaxHeapAttributeNumber + (index) + 1) - -#define EXPRESSION_INDEX(attnum) \ - ((attnum) - MaxHeapAttributeNumber - 1) - #endif /* EXTENDED_STATS_INTERNAL_H */ diff --git a/src/include/statistics/statistics.h b/src/include/statistics/statistics.h index 006d578e0c..326cf26fea 100644 --- a/src/include/statistics/statistics.h +++ b/src/include/statistics/statistics.h @@ -26,7 +26,8 @@ typedef struct MVNDistinctItem { double ndistinct; /* ndistinct value for this combination */ - Bitmapset *attrs; /* attr numbers of items */ + int nattributes; /* number of attributes */ + AttrNumber *attributes; /* attribute numbers */ } MVNDistinctItem; /* A MVNDistinct object, comprising all possible combinations of columns */ -- 2.26.2 --------------614DDB87AFFED893713AC0E9 Content-Type: text/x-patch; charset=UTF-8; name="0005-WIP-unify-handling-of-attributes-and-expres-20210304.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename*0="0005-WIP-unify-handling-of-attributes-and-expres-20210304.pa"; filename*1="tch" ^ permalink raw reply [nested|flat] 8+ messages in thread
* SCRAM pass-through authentication for postgres_fdw @ 2024-12-04 18:44 Matheus Alcantara <[email protected]> 2024-12-04 22:11 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Matheus Alcantara @ 2024-12-04 18:44 UTC (permalink / raw) To: PostgreSQL Hackers <[email protected]> Hi, The attached a patch enables SCRAM authentication for postgres_fdw connections without requiring plain-text password on user mapping properties. This is achieved by storing the SCRAM ClientKey and ServerKey obtained during client authentication with the backend. These keys are then used to complete the SCRAM exchange between the backend and the fdw server, eliminating the need to derive them from a stored plain-text password. I think that some documentation updates may be necessary for this change. If so, I plan to submit an updated patch with the relevant documentation changes in the coming days. This patch is based on a previous WIP patch from Peter Eisentraut [1] [1] https://github.com/petere/postgresql/commit/90009ccd736e99d65c59b9078d14d76fffc2426a -- Matheus Alcantara EDB: https://www.enterprisedb.com From 65fcb8c9565c7f4ba5c204af775c29c76d474a57 Mon Sep 17 00:00:00 2001 From: Matheus Alcantara <[email protected]> Date: Tue, 19 Nov 2024 15:37:57 -0300 Subject: [PATCH v1] postgres_fdw: SCRAM authentication pass-through This commit enable SCRAM authentication for postgres_fdw when connecting to a fdw server without having to store a plain-text password on user mapping options. This is done by saving the SCRAM ClientKey and ServeryKey from the client authentication and using those instead of the plain-text password for the server-side SCRAM exchange. --- contrib/postgres_fdw/Makefile | 1 + contrib/postgres_fdw/connection.c | 67 ++++++++++++++++++++++-- contrib/postgres_fdw/meson.build | 5 ++ contrib/postgres_fdw/option.c | 3 ++ contrib/postgres_fdw/t/001_auth_scram.pl | 62 ++++++++++++++++++++++ src/backend/libpq/auth-scram.c | 16 ++++-- src/include/libpq/libpq-be.h | 9 ++++ src/interfaces/libpq/fe-auth-scram.c | 29 ++++++++-- src/interfaces/libpq/fe-auth.c | 2 +- src/interfaces/libpq/fe-connect.c | 31 +++++++++++ src/interfaces/libpq/libpq-int.h | 6 +++ 11 files changed, 217 insertions(+), 14 deletions(-) create mode 100644 contrib/postgres_fdw/t/001_auth_scram.pl diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile index 88fdce40d6..6c12c8e925 100644 --- a/contrib/postgres_fdw/Makefile +++ b/contrib/postgres_fdw/Makefile @@ -8,6 +8,7 @@ OBJS = \ option.o \ postgres_fdw.o \ shippable.o +TAP_TESTS = 1 PGFILEDESC = "postgres_fdw - foreign data wrapper for PostgreSQL" PG_CPPFLAGS = -I$(libpq_srcdir) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 2326f391d3..e0e1ebe0d4 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -19,6 +19,7 @@ #include "access/xact.h" #include "catalog/pg_user_mapping.h" #include "commands/defrem.h" +#include "common/base64.h" #include "funcapi.h" #include "libpq/libpq-be.h" #include "libpq/libpq-be-fe-helpers.h" @@ -168,6 +169,7 @@ static void pgfdw_finish_abort_cleanup(List *pending_entries, static void pgfdw_security_check(const char **keywords, const char **values, UserMapping *user, PGconn *conn); static bool UserMappingPasswordRequired(UserMapping *user); +static bool UseScramPassthrough(ForeignServer *server, UserMapping *user); static bool disconnect_cached_connections(Oid serverid); static void postgres_fdw_get_connections_internal(FunctionCallInfo fcinfo, enum pgfdwVersion api_version); @@ -476,7 +478,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user) * for application_name, fallback_application_name, client_encoding, * end marker. */ - n = list_length(server->options) + list_length(user->options) + 4; + n = list_length(server->options) + list_length(user->options) + 4 + 2; keywords = (const char **) palloc(n * sizeof(char *)); values = (const char **) palloc(n * sizeof(char *)); @@ -545,10 +547,37 @@ connect_pg_server(ForeignServer *server, UserMapping *user) values[n] = GetDatabaseEncodingName(); n++; + if (MyProcPort->has_scram_keys && UseScramPassthrough(server, user)) + { + int len; + + keywords[n] = "scram_client_key"; + len = pg_b64_enc_len(sizeof(MyProcPort->scram_ClientKey)); + /* don't forget the zero-terminator */ + values[n] = palloc0(len+1); + pg_b64_encode((const char *) MyProcPort->scram_ClientKey, + sizeof(MyProcPort->scram_ClientKey), + (char *) values[n], len); + n++; + + keywords[n] = "scram_server_key"; + len = pg_b64_enc_len(sizeof(MyProcPort->scram_ServerKey)); + /* don't forget the zero-terminator */ + values[n] = palloc0(len+1); + pg_b64_encode((const char *) MyProcPort->scram_ServerKey, + sizeof(MyProcPort->scram_ServerKey), + (char *) values[n], len); + n++; + } + keywords[n] = values[n] = NULL; - /* verify the set of connection parameters */ - check_conn_params(keywords, values, user); + /* + * Verify the set of connection parameters only if scram pass-through + * is not being used because the password is not necessary. + */ + if (!(MyProcPort->has_scram_keys && UseScramPassthrough(server, user))) + check_conn_params(keywords, values, user); /* first time, allocate or get the custom wait event */ if (pgfdw_we_connect == 0) @@ -566,8 +595,12 @@ connect_pg_server(ForeignServer *server, UserMapping *user) server->servername), errdetail_internal("%s", pchomp(PQerrorMessage(conn))))); - /* Perform post-connection security checks */ - pgfdw_security_check(keywords, values, user, conn); + /* + * Perform post-connection security checks only if scram pass-through + * is not being used because the password is not necessary. + */ + if (!(MyProcPort->has_scram_keys && UseScramPassthrough(server, user))) + pgfdw_security_check(keywords, values, user, conn); /* Prepare new session for use */ configure_remote_session(conn); @@ -620,6 +653,30 @@ UserMappingPasswordRequired(UserMapping *user) return true; } +static bool +UseScramPassthrough(ForeignServer *server, UserMapping *user) +{ + ListCell *cell; + + foreach(cell, server->options) + { + DefElem *def = (DefElem *) lfirst(cell); + + if (strcmp(def->defname, "use_scram_passthrough") == 0) + return defGetBoolean(def); + } + + foreach(cell, user->options) + { + DefElem *def = (DefElem *) lfirst(cell); + + if (strcmp(def->defname, "use_scram_passthrough") == 0) + return defGetBoolean(def); + } + + return false; +} + /* * For non-superusers, insist that the connstr specify a password or that the * user provided their own GSSAPI delegated credentials. This diff --git a/contrib/postgres_fdw/meson.build b/contrib/postgres_fdw/meson.build index 3014086ba6..27d07188fc 100644 --- a/contrib/postgres_fdw/meson.build +++ b/contrib/postgres_fdw/meson.build @@ -41,4 +41,9 @@ tests += { ], 'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'], }, + 'tap': { + 'tests': [ + 't/001_auth_scram.pl', + ], + }, } diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 232d85354b..15abc64381 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -279,6 +279,9 @@ InitPgFdwOptions(void) {"analyze_sampling", ForeignServerRelationId, false}, {"analyze_sampling", ForeignTableRelationId, false}, + {"use_scram_passthrough", ForeignServerRelationId, false}, + {"use_scram_passthrough", UserMappingRelationId, false}, + /* * sslcert and sslkey are in fact libpq options, but we repeat them * here to allow them to appear in both foreign server context (when diff --git a/contrib/postgres_fdw/t/001_auth_scram.pl b/contrib/postgres_fdw/t/001_auth_scram.pl new file mode 100644 index 0000000000..388d2179db --- /dev/null +++ b/contrib/postgres_fdw/t/001_auth_scram.pl @@ -0,0 +1,62 @@ +# Copyright (c) 2021-2024, PostgreSQL Global Development Group + +# Test SCRAM authentication pass through the intermediary postgres_fdw to the server + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Utils; +use PostgreSQL::Test::Cluster; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('node'); +my $hostaddr = '127.0.0.1'; +my $user = "user01"; +my $db1 = "db1"; +my $db2 = "db2"; +my $fdw_server = "db2_fdw"; +my $host = $node->host; +my $port = $node->port; +my $connstr = $node->connstr($db1) . qq' user=$user'; + +$node->init; +$node->start; + +# Test setup + +$node->safe_psql('postgres', qq'CREATE USER $user WITH password \'pass\' '); +$node->safe_psql('postgres', qq'CREATE DATABASE $db1'); +$node->safe_psql('postgres', qq'CREATE DATABASE $db2'); + +$node->safe_psql($db2, 'CREATE TABLE t AS SELECT g,g+1 FROM generate_series(1,10) g(g)'); +$node->safe_psql($db2, qq'GRANT USAGE ON SCHEMA public to $user'); +$node->safe_psql($db2, qq'GRANT SELECT ON t to $user'); + +$node->safe_psql($db1, 'CREATE EXTENSION IF NOT EXISTS postgres_fdw'); +$node->safe_psql($db1, qq'CREATE SERVER $fdw_server FOREIGN DATA WRAPPER postgres_fdw options ( + host \'$host\', port \'$port\', dbname \'$db2\', use_scram_passthrough \'true\') '); +# password not required +$node->safe_psql($db1, qq'CREATE USER MAPPING FOR $user SERVER $fdw_server OPTIONS (user \'$user\');'); +$node->safe_psql($db1, qq'GRANT USAGE ON FOREIGN SERVER $fdw_server to $user;'); +$node->safe_psql($db1, qq'GRANT ALL ON SCHEMA public to $user'); + +unlink($node->data_dir . '/pg_hba.conf'); +$node->append_conf( + 'pg_hba.conf', qq{ +local all all scram-sha-256 +host all all $hostaddr/32 scram-sha-256 +}); +$node->restart; + +# End of test setup + +$ENV{PGPASSWORD} = "pass"; + +$node->safe_psql($db1, qq'IMPORT FOREIGN SCHEMA public LIMIT TO(t) FROM SERVER $fdw_server INTO public ;', + connstr=>$connstr); + +my $ret = $node->safe_psql($db1, 'SELECT count(1) FROM t', + connstr=>$connstr); +is($ret, '10', 'SELECT count from fdw server returns 10'); + + +done_testing(); diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c index 8c5b6d9c67..88a15cc0e5 100644 --- a/src/backend/libpq/auth-scram.c +++ b/src/backend/libpq/auth-scram.c @@ -101,6 +101,7 @@ #include "libpq/crypt.h" #include "libpq/sasl.h" #include "libpq/scram.h" +#include "miscadmin.h" static void scram_get_mechanisms(Port *port, StringInfo buf); static void *scram_init(Port *port, const char *selected_mech, @@ -144,6 +145,7 @@ typedef struct int iterations; char *salt; /* base64-encoded */ + uint8 ClientKey[SCRAM_MAX_KEY_LEN]; uint8 StoredKey[SCRAM_MAX_KEY_LEN]; uint8 ServerKey[SCRAM_MAX_KEY_LEN]; @@ -462,6 +464,13 @@ scram_exchange(void *opaq, const char *input, int inputlen, if (*output) *outputlen = strlen(*output); + if (result == PG_SASL_EXCHANGE_SUCCESS && state->state == SCRAM_AUTH_FINISHED) + { + memcpy(MyProcPort->scram_ClientKey, state->ClientKey, sizeof(MyProcPort->scram_ClientKey)); + memcpy(MyProcPort->scram_ServerKey, state->ServerKey, sizeof(MyProcPort->scram_ServerKey)); + MyProcPort->has_scram_keys = true; + } + return result; } @@ -1140,9 +1149,8 @@ static bool verify_client_proof(scram_state *state) { uint8 ClientSignature[SCRAM_MAX_KEY_LEN]; - uint8 ClientKey[SCRAM_MAX_KEY_LEN]; uint8 client_StoredKey[SCRAM_MAX_KEY_LEN]; - pg_hmac_ctx *ctx = pg_hmac_create(state->hash_type); + pg_hmac_ctx *ctx = pg_hmac_create(PG_SHA256); int i; const char *errstr = NULL; @@ -1173,10 +1181,10 @@ verify_client_proof(scram_state *state) /* Extract the ClientKey that the client calculated from the proof */ for (i = 0; i < state->key_length; i++) - ClientKey[i] = state->ClientProof[i] ^ ClientSignature[i]; + state->ClientKey[i] = state->ClientProof[i] ^ ClientSignature[i]; /* Hash it one more time, and compare with StoredKey */ - if (scram_H(ClientKey, state->hash_type, state->key_length, + if (scram_H(state->ClientKey, state->hash_type, state->key_length, client_StoredKey, &errstr) < 0) elog(ERROR, "could not hash stored key: %s", errstr); diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h index 9109b2c334..4eb9e80523 100644 --- a/src/include/libpq/libpq-be.h +++ b/src/include/libpq/libpq-be.h @@ -18,6 +18,8 @@ #ifndef LIBPQ_BE_H #define LIBPQ_BE_H +#include "common/scram-common.h" + #include <sys/time.h> #ifdef USE_OPENSSL #include <openssl/ssl.h> @@ -181,6 +183,13 @@ typedef struct Port int keepalives_count; int tcp_user_timeout; + /* + * SCRAM structures. + */ + uint8 scram_ClientKey[SCRAM_MAX_KEY_LEN]; + uint8 scram_ServerKey[SCRAM_MAX_KEY_LEN]; + bool has_scram_keys; /* true if the above two are valid */ + /* * GSSAPI structures. */ diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c index 0bb820e0d9..7beb5a9d31 100644 --- a/src/interfaces/libpq/fe-auth-scram.c +++ b/src/interfaces/libpq/fe-auth-scram.c @@ -119,6 +119,8 @@ scram_init(PGconn *conn, return NULL; } + if (password) + { /* Normalize the password with SASLprep, if possible */ rc = pg_saslprep(password, &prep_password); if (rc == SASLPREP_OOM) @@ -138,6 +140,7 @@ scram_init(PGconn *conn, } } state->password = prep_password; + } return state; } @@ -775,6 +778,12 @@ calculate_client_proof(fe_scram_state *state, return false; } + if (state->conn->scram_client_key_binary) + { + memcpy(ClientKey, state->conn->scram_client_key_binary, SCRAM_MAX_KEY_LEN); + } + else + { /* * Calculate SaltedPassword, and store it in 'state' so that we can reuse * it later in verify_server_signature. @@ -783,15 +792,20 @@ calculate_client_proof(fe_scram_state *state, state->key_length, state->salt, state->saltlen, state->iterations, state->SaltedPassword, errstr) < 0 || - scram_ClientKey(state->SaltedPassword, state->hash_type, - state->key_length, ClientKey, errstr) < 0 || - scram_H(ClientKey, state->hash_type, state->key_length, - StoredKey, errstr) < 0) + scram_ClientKey(state->SaltedPassword, state->hash_type, + state->key_length, ClientKey, errstr) < 0) { /* errstr is already filled here */ pg_hmac_free(ctx); return false; } + } + + if (scram_H(ClientKey, state->hash_type, state->key_length, StoredKey, errstr) < 0) + { + pg_hmac_free(ctx); + return false; + } if (pg_hmac_init(ctx, StoredKey, state->key_length) < 0 || pg_hmac_update(ctx, @@ -841,6 +855,12 @@ verify_server_signature(fe_scram_state *state, bool *match, return false; } + if (state->conn->scram_server_key_binary) + { + memcpy(ServerKey, state->conn->scram_server_key_binary, SCRAM_MAX_KEY_LEN); + } + else + { if (scram_ServerKey(state->SaltedPassword, state->hash_type, state->key_length, ServerKey, errstr) < 0) { @@ -848,6 +868,7 @@ verify_server_signature(fe_scram_state *state, bool *match, pg_hmac_free(ctx); return false; } + } /* calculate ServerSignature */ if (pg_hmac_init(ctx, ServerKey, state->key_length) < 0 || diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 20d3427e94..ef1c965cd5 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -559,7 +559,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) * First, select the password to use for the exchange, complaining if * there isn't one and the selected SASL mechanism needs it. */ - if (conn->password_needed) + if (conn->password_needed && !conn->scram_client_key_binary) { password = conn->connhost[conn->whichhost].password; if (password == NULL) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index aaf87e8e88..464cefd901 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -22,6 +22,7 @@ #include <time.h> #include <unistd.h> +#include "common/base64.h" #include "common/ip.h" #include "common/link-canary.h" #include "common/scram-common.h" @@ -365,6 +366,12 @@ static const internalPQconninfoOption PQconninfoOptions[] = { "Load-Balance-Hosts", "", 8, /* sizeof("disable") = 8 */ offsetof(struct pg_conn, load_balance_hosts)}, + {"scram_client_key", NULL, NULL, NULL, "SCRAM-Client-Key", "D", SCRAM_MAX_KEY_LEN * 2, + offsetof(struct pg_conn, scram_client_key)}, + + {"scram_server_key", NULL, NULL, NULL, "SCRAM-Server-Key", "D", SCRAM_MAX_KEY_LEN * 2, + offsetof(struct pg_conn, scram_server_key)}, + /* Terminating entry --- MUST BE LAST */ {NULL, NULL, NULL, NULL, NULL, NULL, 0} @@ -1792,6 +1799,28 @@ pqConnectOptions2(PGconn *conn) else conn->target_server_type = SERVER_TYPE_ANY; + if (conn->scram_client_key) + { + int len; + + len = pg_b64_dec_len(strlen(conn->scram_client_key)); + conn->scram_client_key_len = len; + conn->scram_client_key_binary = malloc(len); + pg_b64_decode(conn->scram_client_key, strlen(conn->scram_client_key), + conn->scram_client_key_binary, len); + } + + if (conn->scram_server_key) + { + int len; + + len = pg_b64_dec_len(strlen(conn->scram_server_key)); + conn->scram_server_key_len = len; + conn->scram_server_key_binary = malloc(len); + pg_b64_decode(conn->scram_server_key, strlen(conn->scram_server_key), + conn->scram_server_key_binary, len); + } + /* * validate load_balance_hosts option, and set load_balance_type */ @@ -4703,6 +4732,8 @@ freePGconn(PGconn *conn) free(conn->rowBuf); free(conn->target_session_attrs); free(conn->load_balance_hosts); + free(conn->scram_client_key); + free(conn->scram_server_key); termPQExpBuffer(&conn->errorMessage); termPQExpBuffer(&conn->workBuffer); diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 08cc391cbd..17b81c81f4 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -427,6 +427,8 @@ struct pg_conn char *target_session_attrs; /* desired session properties */ char *require_auth; /* name of the expected auth method */ char *load_balance_hosts; /* load balance over hosts */ + char *scram_client_key; + char *scram_server_key; bool cancelRequest; /* true if this connection is used to send a * cancel request, instead of being a normal @@ -517,6 +519,10 @@ struct pg_conn AddrInfo *addr; /* the array of addresses for the currently * tried host */ bool send_appname; /* okay to send application_name? */ + size_t scram_client_key_len; + char *scram_client_key_binary; + size_t scram_server_key_len; + char *scram_server_key_binary; /* Miscellaneous stuff */ int be_pid; /* PID of backend --- needed for cancels */ -- 2.39.3 (Apple Git-146) Attachments: [text/plain] v1-0001-postgres_fdw-SCRAM-authentication-pass-through.patch (17.5K, ../../[email protected]/2-v1-0001-postgres_fdw-SCRAM-authentication-pass-through.patch) download | inline diff: From 65fcb8c9565c7f4ba5c204af775c29c76d474a57 Mon Sep 17 00:00:00 2001 From: Matheus Alcantara <[email protected]> Date: Tue, 19 Nov 2024 15:37:57 -0300 Subject: [PATCH v1] postgres_fdw: SCRAM authentication pass-through This commit enable SCRAM authentication for postgres_fdw when connecting to a fdw server without having to store a plain-text password on user mapping options. This is done by saving the SCRAM ClientKey and ServeryKey from the client authentication and using those instead of the plain-text password for the server-side SCRAM exchange. --- contrib/postgres_fdw/Makefile | 1 + contrib/postgres_fdw/connection.c | 67 ++++++++++++++++++++++-- contrib/postgres_fdw/meson.build | 5 ++ contrib/postgres_fdw/option.c | 3 ++ contrib/postgres_fdw/t/001_auth_scram.pl | 62 ++++++++++++++++++++++ src/backend/libpq/auth-scram.c | 16 ++++-- src/include/libpq/libpq-be.h | 9 ++++ src/interfaces/libpq/fe-auth-scram.c | 29 ++++++++-- src/interfaces/libpq/fe-auth.c | 2 +- src/interfaces/libpq/fe-connect.c | 31 +++++++++++ src/interfaces/libpq/libpq-int.h | 6 +++ 11 files changed, 217 insertions(+), 14 deletions(-) create mode 100644 contrib/postgres_fdw/t/001_auth_scram.pl diff --git a/contrib/postgres_fdw/Makefile b/contrib/postgres_fdw/Makefile index 88fdce40d6..6c12c8e925 100644 --- a/contrib/postgres_fdw/Makefile +++ b/contrib/postgres_fdw/Makefile @@ -8,6 +8,7 @@ OBJS = \ option.o \ postgres_fdw.o \ shippable.o +TAP_TESTS = 1 PGFILEDESC = "postgres_fdw - foreign data wrapper for PostgreSQL" PG_CPPFLAGS = -I$(libpq_srcdir) diff --git a/contrib/postgres_fdw/connection.c b/contrib/postgres_fdw/connection.c index 2326f391d3..e0e1ebe0d4 100644 --- a/contrib/postgres_fdw/connection.c +++ b/contrib/postgres_fdw/connection.c @@ -19,6 +19,7 @@ #include "access/xact.h" #include "catalog/pg_user_mapping.h" #include "commands/defrem.h" +#include "common/base64.h" #include "funcapi.h" #include "libpq/libpq-be.h" #include "libpq/libpq-be-fe-helpers.h" @@ -168,6 +169,7 @@ static void pgfdw_finish_abort_cleanup(List *pending_entries, static void pgfdw_security_check(const char **keywords, const char **values, UserMapping *user, PGconn *conn); static bool UserMappingPasswordRequired(UserMapping *user); +static bool UseScramPassthrough(ForeignServer *server, UserMapping *user); static bool disconnect_cached_connections(Oid serverid); static void postgres_fdw_get_connections_internal(FunctionCallInfo fcinfo, enum pgfdwVersion api_version); @@ -476,7 +478,7 @@ connect_pg_server(ForeignServer *server, UserMapping *user) * for application_name, fallback_application_name, client_encoding, * end marker. */ - n = list_length(server->options) + list_length(user->options) + 4; + n = list_length(server->options) + list_length(user->options) + 4 + 2; keywords = (const char **) palloc(n * sizeof(char *)); values = (const char **) palloc(n * sizeof(char *)); @@ -545,10 +547,37 @@ connect_pg_server(ForeignServer *server, UserMapping *user) values[n] = GetDatabaseEncodingName(); n++; + if (MyProcPort->has_scram_keys && UseScramPassthrough(server, user)) + { + int len; + + keywords[n] = "scram_client_key"; + len = pg_b64_enc_len(sizeof(MyProcPort->scram_ClientKey)); + /* don't forget the zero-terminator */ + values[n] = palloc0(len+1); + pg_b64_encode((const char *) MyProcPort->scram_ClientKey, + sizeof(MyProcPort->scram_ClientKey), + (char *) values[n], len); + n++; + + keywords[n] = "scram_server_key"; + len = pg_b64_enc_len(sizeof(MyProcPort->scram_ServerKey)); + /* don't forget the zero-terminator */ + values[n] = palloc0(len+1); + pg_b64_encode((const char *) MyProcPort->scram_ServerKey, + sizeof(MyProcPort->scram_ServerKey), + (char *) values[n], len); + n++; + } + keywords[n] = values[n] = NULL; - /* verify the set of connection parameters */ - check_conn_params(keywords, values, user); + /* + * Verify the set of connection parameters only if scram pass-through + * is not being used because the password is not necessary. + */ + if (!(MyProcPort->has_scram_keys && UseScramPassthrough(server, user))) + check_conn_params(keywords, values, user); /* first time, allocate or get the custom wait event */ if (pgfdw_we_connect == 0) @@ -566,8 +595,12 @@ connect_pg_server(ForeignServer *server, UserMapping *user) server->servername), errdetail_internal("%s", pchomp(PQerrorMessage(conn))))); - /* Perform post-connection security checks */ - pgfdw_security_check(keywords, values, user, conn); + /* + * Perform post-connection security checks only if scram pass-through + * is not being used because the password is not necessary. + */ + if (!(MyProcPort->has_scram_keys && UseScramPassthrough(server, user))) + pgfdw_security_check(keywords, values, user, conn); /* Prepare new session for use */ configure_remote_session(conn); @@ -620,6 +653,30 @@ UserMappingPasswordRequired(UserMapping *user) return true; } +static bool +UseScramPassthrough(ForeignServer *server, UserMapping *user) +{ + ListCell *cell; + + foreach(cell, server->options) + { + DefElem *def = (DefElem *) lfirst(cell); + + if (strcmp(def->defname, "use_scram_passthrough") == 0) + return defGetBoolean(def); + } + + foreach(cell, user->options) + { + DefElem *def = (DefElem *) lfirst(cell); + + if (strcmp(def->defname, "use_scram_passthrough") == 0) + return defGetBoolean(def); + } + + return false; +} + /* * For non-superusers, insist that the connstr specify a password or that the * user provided their own GSSAPI delegated credentials. This diff --git a/contrib/postgres_fdw/meson.build b/contrib/postgres_fdw/meson.build index 3014086ba6..27d07188fc 100644 --- a/contrib/postgres_fdw/meson.build +++ b/contrib/postgres_fdw/meson.build @@ -41,4 +41,9 @@ tests += { ], 'regress_args': ['--dlpath', meson.build_root() / 'src/test/regress'], }, + 'tap': { + 'tests': [ + 't/001_auth_scram.pl', + ], + }, } diff --git a/contrib/postgres_fdw/option.c b/contrib/postgres_fdw/option.c index 232d85354b..15abc64381 100644 --- a/contrib/postgres_fdw/option.c +++ b/contrib/postgres_fdw/option.c @@ -279,6 +279,9 @@ InitPgFdwOptions(void) {"analyze_sampling", ForeignServerRelationId, false}, {"analyze_sampling", ForeignTableRelationId, false}, + {"use_scram_passthrough", ForeignServerRelationId, false}, + {"use_scram_passthrough", UserMappingRelationId, false}, + /* * sslcert and sslkey are in fact libpq options, but we repeat them * here to allow them to appear in both foreign server context (when diff --git a/contrib/postgres_fdw/t/001_auth_scram.pl b/contrib/postgres_fdw/t/001_auth_scram.pl new file mode 100644 index 0000000000..388d2179db --- /dev/null +++ b/contrib/postgres_fdw/t/001_auth_scram.pl @@ -0,0 +1,62 @@ +# Copyright (c) 2021-2024, PostgreSQL Global Development Group + +# Test SCRAM authentication pass through the intermediary postgres_fdw to the server + +use strict; +use warnings FATAL => 'all'; +use PostgreSQL::Test::Utils; +use PostgreSQL::Test::Cluster; +use Test::More; + +my $node = PostgreSQL::Test::Cluster->new('node'); +my $hostaddr = '127.0.0.1'; +my $user = "user01"; +my $db1 = "db1"; +my $db2 = "db2"; +my $fdw_server = "db2_fdw"; +my $host = $node->host; +my $port = $node->port; +my $connstr = $node->connstr($db1) . qq' user=$user'; + +$node->init; +$node->start; + +# Test setup + +$node->safe_psql('postgres', qq'CREATE USER $user WITH password \'pass\' '); +$node->safe_psql('postgres', qq'CREATE DATABASE $db1'); +$node->safe_psql('postgres', qq'CREATE DATABASE $db2'); + +$node->safe_psql($db2, 'CREATE TABLE t AS SELECT g,g+1 FROM generate_series(1,10) g(g)'); +$node->safe_psql($db2, qq'GRANT USAGE ON SCHEMA public to $user'); +$node->safe_psql($db2, qq'GRANT SELECT ON t to $user'); + +$node->safe_psql($db1, 'CREATE EXTENSION IF NOT EXISTS postgres_fdw'); +$node->safe_psql($db1, qq'CREATE SERVER $fdw_server FOREIGN DATA WRAPPER postgres_fdw options ( + host \'$host\', port \'$port\', dbname \'$db2\', use_scram_passthrough \'true\') '); +# password not required +$node->safe_psql($db1, qq'CREATE USER MAPPING FOR $user SERVER $fdw_server OPTIONS (user \'$user\');'); +$node->safe_psql($db1, qq'GRANT USAGE ON FOREIGN SERVER $fdw_server to $user;'); +$node->safe_psql($db1, qq'GRANT ALL ON SCHEMA public to $user'); + +unlink($node->data_dir . '/pg_hba.conf'); +$node->append_conf( + 'pg_hba.conf', qq{ +local all all scram-sha-256 +host all all $hostaddr/32 scram-sha-256 +}); +$node->restart; + +# End of test setup + +$ENV{PGPASSWORD} = "pass"; + +$node->safe_psql($db1, qq'IMPORT FOREIGN SCHEMA public LIMIT TO(t) FROM SERVER $fdw_server INTO public ;', + connstr=>$connstr); + +my $ret = $node->safe_psql($db1, 'SELECT count(1) FROM t', + connstr=>$connstr); +is($ret, '10', 'SELECT count from fdw server returns 10'); + + +done_testing(); diff --git a/src/backend/libpq/auth-scram.c b/src/backend/libpq/auth-scram.c index 8c5b6d9c67..88a15cc0e5 100644 --- a/src/backend/libpq/auth-scram.c +++ b/src/backend/libpq/auth-scram.c @@ -101,6 +101,7 @@ #include "libpq/crypt.h" #include "libpq/sasl.h" #include "libpq/scram.h" +#include "miscadmin.h" static void scram_get_mechanisms(Port *port, StringInfo buf); static void *scram_init(Port *port, const char *selected_mech, @@ -144,6 +145,7 @@ typedef struct int iterations; char *salt; /* base64-encoded */ + uint8 ClientKey[SCRAM_MAX_KEY_LEN]; uint8 StoredKey[SCRAM_MAX_KEY_LEN]; uint8 ServerKey[SCRAM_MAX_KEY_LEN]; @@ -462,6 +464,13 @@ scram_exchange(void *opaq, const char *input, int inputlen, if (*output) *outputlen = strlen(*output); + if (result == PG_SASL_EXCHANGE_SUCCESS && state->state == SCRAM_AUTH_FINISHED) + { + memcpy(MyProcPort->scram_ClientKey, state->ClientKey, sizeof(MyProcPort->scram_ClientKey)); + memcpy(MyProcPort->scram_ServerKey, state->ServerKey, sizeof(MyProcPort->scram_ServerKey)); + MyProcPort->has_scram_keys = true; + } + return result; } @@ -1140,9 +1149,8 @@ static bool verify_client_proof(scram_state *state) { uint8 ClientSignature[SCRAM_MAX_KEY_LEN]; - uint8 ClientKey[SCRAM_MAX_KEY_LEN]; uint8 client_StoredKey[SCRAM_MAX_KEY_LEN]; - pg_hmac_ctx *ctx = pg_hmac_create(state->hash_type); + pg_hmac_ctx *ctx = pg_hmac_create(PG_SHA256); int i; const char *errstr = NULL; @@ -1173,10 +1181,10 @@ verify_client_proof(scram_state *state) /* Extract the ClientKey that the client calculated from the proof */ for (i = 0; i < state->key_length; i++) - ClientKey[i] = state->ClientProof[i] ^ ClientSignature[i]; + state->ClientKey[i] = state->ClientProof[i] ^ ClientSignature[i]; /* Hash it one more time, and compare with StoredKey */ - if (scram_H(ClientKey, state->hash_type, state->key_length, + if (scram_H(state->ClientKey, state->hash_type, state->key_length, client_StoredKey, &errstr) < 0) elog(ERROR, "could not hash stored key: %s", errstr); diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h index 9109b2c334..4eb9e80523 100644 --- a/src/include/libpq/libpq-be.h +++ b/src/include/libpq/libpq-be.h @@ -18,6 +18,8 @@ #ifndef LIBPQ_BE_H #define LIBPQ_BE_H +#include "common/scram-common.h" + #include <sys/time.h> #ifdef USE_OPENSSL #include <openssl/ssl.h> @@ -181,6 +183,13 @@ typedef struct Port int keepalives_count; int tcp_user_timeout; + /* + * SCRAM structures. + */ + uint8 scram_ClientKey[SCRAM_MAX_KEY_LEN]; + uint8 scram_ServerKey[SCRAM_MAX_KEY_LEN]; + bool has_scram_keys; /* true if the above two are valid */ + /* * GSSAPI structures. */ diff --git a/src/interfaces/libpq/fe-auth-scram.c b/src/interfaces/libpq/fe-auth-scram.c index 0bb820e0d9..7beb5a9d31 100644 --- a/src/interfaces/libpq/fe-auth-scram.c +++ b/src/interfaces/libpq/fe-auth-scram.c @@ -119,6 +119,8 @@ scram_init(PGconn *conn, return NULL; } + if (password) + { /* Normalize the password with SASLprep, if possible */ rc = pg_saslprep(password, &prep_password); if (rc == SASLPREP_OOM) @@ -138,6 +140,7 @@ scram_init(PGconn *conn, } } state->password = prep_password; + } return state; } @@ -775,6 +778,12 @@ calculate_client_proof(fe_scram_state *state, return false; } + if (state->conn->scram_client_key_binary) + { + memcpy(ClientKey, state->conn->scram_client_key_binary, SCRAM_MAX_KEY_LEN); + } + else + { /* * Calculate SaltedPassword, and store it in 'state' so that we can reuse * it later in verify_server_signature. @@ -783,15 +792,20 @@ calculate_client_proof(fe_scram_state *state, state->key_length, state->salt, state->saltlen, state->iterations, state->SaltedPassword, errstr) < 0 || - scram_ClientKey(state->SaltedPassword, state->hash_type, - state->key_length, ClientKey, errstr) < 0 || - scram_H(ClientKey, state->hash_type, state->key_length, - StoredKey, errstr) < 0) + scram_ClientKey(state->SaltedPassword, state->hash_type, + state->key_length, ClientKey, errstr) < 0) { /* errstr is already filled here */ pg_hmac_free(ctx); return false; } + } + + if (scram_H(ClientKey, state->hash_type, state->key_length, StoredKey, errstr) < 0) + { + pg_hmac_free(ctx); + return false; + } if (pg_hmac_init(ctx, StoredKey, state->key_length) < 0 || pg_hmac_update(ctx, @@ -841,6 +855,12 @@ verify_server_signature(fe_scram_state *state, bool *match, return false; } + if (state->conn->scram_server_key_binary) + { + memcpy(ServerKey, state->conn->scram_server_key_binary, SCRAM_MAX_KEY_LEN); + } + else + { if (scram_ServerKey(state->SaltedPassword, state->hash_type, state->key_length, ServerKey, errstr) < 0) { @@ -848,6 +868,7 @@ verify_server_signature(fe_scram_state *state, bool *match, pg_hmac_free(ctx); return false; } + } /* calculate ServerSignature */ if (pg_hmac_init(ctx, ServerKey, state->key_length) < 0 || diff --git a/src/interfaces/libpq/fe-auth.c b/src/interfaces/libpq/fe-auth.c index 20d3427e94..ef1c965cd5 100644 --- a/src/interfaces/libpq/fe-auth.c +++ b/src/interfaces/libpq/fe-auth.c @@ -559,7 +559,7 @@ pg_SASL_init(PGconn *conn, int payloadlen) * First, select the password to use for the exchange, complaining if * there isn't one and the selected SASL mechanism needs it. */ - if (conn->password_needed) + if (conn->password_needed && !conn->scram_client_key_binary) { password = conn->connhost[conn->whichhost].password; if (password == NULL) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index aaf87e8e88..464cefd901 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -22,6 +22,7 @@ #include <time.h> #include <unistd.h> +#include "common/base64.h" #include "common/ip.h" #include "common/link-canary.h" #include "common/scram-common.h" @@ -365,6 +366,12 @@ static const internalPQconninfoOption PQconninfoOptions[] = { "Load-Balance-Hosts", "", 8, /* sizeof("disable") = 8 */ offsetof(struct pg_conn, load_balance_hosts)}, + {"scram_client_key", NULL, NULL, NULL, "SCRAM-Client-Key", "D", SCRAM_MAX_KEY_LEN * 2, + offsetof(struct pg_conn, scram_client_key)}, + + {"scram_server_key", NULL, NULL, NULL, "SCRAM-Server-Key", "D", SCRAM_MAX_KEY_LEN * 2, + offsetof(struct pg_conn, scram_server_key)}, + /* Terminating entry --- MUST BE LAST */ {NULL, NULL, NULL, NULL, NULL, NULL, 0} @@ -1792,6 +1799,28 @@ pqConnectOptions2(PGconn *conn) else conn->target_server_type = SERVER_TYPE_ANY; + if (conn->scram_client_key) + { + int len; + + len = pg_b64_dec_len(strlen(conn->scram_client_key)); + conn->scram_client_key_len = len; + conn->scram_client_key_binary = malloc(len); + pg_b64_decode(conn->scram_client_key, strlen(conn->scram_client_key), + conn->scram_client_key_binary, len); + } + + if (conn->scram_server_key) + { + int len; + + len = pg_b64_dec_len(strlen(conn->scram_server_key)); + conn->scram_server_key_len = len; + conn->scram_server_key_binary = malloc(len); + pg_b64_decode(conn->scram_server_key, strlen(conn->scram_server_key), + conn->scram_server_key_binary, len); + } + /* * validate load_balance_hosts option, and set load_balance_type */ @@ -4703,6 +4732,8 @@ freePGconn(PGconn *conn) free(conn->rowBuf); free(conn->target_session_attrs); free(conn->load_balance_hosts); + free(conn->scram_client_key); + free(conn->scram_server_key); termPQExpBuffer(&conn->errorMessage); termPQExpBuffer(&conn->workBuffer); diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 08cc391cbd..17b81c81f4 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -427,6 +427,8 @@ struct pg_conn char *target_session_attrs; /* desired session properties */ char *require_auth; /* name of the expected auth method */ char *load_balance_hosts; /* load balance over hosts */ + char *scram_client_key; + char *scram_server_key; bool cancelRequest; /* true if this connection is used to send a * cancel request, instead of being a normal @@ -517,6 +519,10 @@ struct pg_conn AddrInfo *addr; /* the array of addresses for the currently * tried host */ bool send_appname; /* okay to send application_name? */ + size_t scram_client_key_len; + char *scram_client_key_binary; + size_t scram_server_key_len; + char *scram_server_key_binary; /* Miscellaneous stuff */ int be_pid; /* PID of backend --- needed for cancels */ -- 2.39.3 (Apple Git-146) ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: SCRAM pass-through authentication for postgres_fdw 2024-12-04 18:44 SCRAM pass-through authentication for postgres_fdw Matheus Alcantara <[email protected]> @ 2024-12-04 22:11 ` Jacob Champion <[email protected]> 2024-12-04 23:05 ` Re: SCRAM pass-through authentication for postgres_fdw Jelte Fennema-Nio <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Jacob Champion @ 2024-12-04 22:11 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]> On Wed, Dec 4, 2024 at 10:45 AM Matheus Alcantara <[email protected]> wrote: > This is achieved by storing the SCRAM ClientKey and ServerKey obtained > during client authentication with the backend. These keys are then > used to complete the SCRAM exchange between the backend and the fdw > server, eliminating the need to derive them from a stored plain-text > password. What are the assumptions that have to be made for pass-through SCRAM to succeed? Is it just "the two servers have identical verifiers for the user," or are there others? It looks like the patch is using the following property [1]: If an attacker obtains the authentication information from the authentication repository and either eavesdrops on one authentication exchange or impersonates a server, the attacker gains the ability to impersonate that user to all servers providing SCRAM access using the same hash function, password, iteration count, and salt. For this reason, it is important to use randomly generated salt values. It makes me a little uneasy to give users a reason to copy identical salts/verifiers around... But for e.g. a loopback connection, it seems like there'd be no additional risk. Is that the target use case? I haven't looked at the code very closely yet, but the following hunk jumped out at me: > - pg_hmac_ctx *ctx = pg_hmac_create(state->hash_type); > + pg_hmac_ctx *ctx = pg_hmac_create(PG_SHA256); Why was that change made? Thanks, --Jacob [1] https://datatracker.ietf.org/doc/html/rfc5802#section-9 ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: SCRAM pass-through authentication for postgres_fdw 2024-12-04 18:44 SCRAM pass-through authentication for postgres_fdw Matheus Alcantara <[email protected]> 2024-12-04 22:11 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> @ 2024-12-04 23:05 ` Jelte Fennema-Nio <[email protected]> 2024-12-04 23:39 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 0 siblings, 1 reply; 8+ messages in thread From: Jelte Fennema-Nio @ 2024-12-04 23:05 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, 4 Dec 2024 at 23:11, Jacob Champion <[email protected]> wrote: > It makes me a little uneasy to give users a reason to copy identical > salts/verifiers around... But for e.g. a loopback connection, it seems > like there'd be no additional risk. Is that the target use case? I don't think that necessarily has to be the usecase, clustering/sharding setups could benefit from this too. PgBouncer supports the same functionality[1]. I only see advantages over the alternative, which is copying the plaintext password around. In case of compromise of the server, only the salt+verifier has to be rotated, not the actual user password. Regarding the actual patch: This definitely needs a bunch of documentation explaining how to use this and when not to use this. ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: SCRAM pass-through authentication for postgres_fdw 2024-12-04 18:44 SCRAM pass-through authentication for postgres_fdw Matheus Alcantara <[email protected]> 2024-12-04 22:11 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 2024-12-04 23:05 ` Re: SCRAM pass-through authentication for postgres_fdw Jelte Fennema-Nio <[email protected]> @ 2024-12-04 23:39 ` Jacob Champion <[email protected]> 2024-12-04 23:49 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 2024-12-11 19:04 ` Re: SCRAM pass-through authentication for postgres_fdw Matheus Alcantara <[email protected]> 0 siblings, 2 replies; 8+ messages in thread From: Jacob Champion @ 2024-12-04 23:39 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Dec 4, 2024 at 3:05 PM Jelte Fennema-Nio <[email protected]> wrote: > I only see advantages over the > alternative, which is copying the plaintext password around. In case > of compromise of the server, only the salt+verifier has to be rotated, > not the actual user password. Sure, I'm not saying it's worse than plaintext. But a third alternative might be actual pass-through SCRAM [1], where either you expect the two servers to share a certificate fingerprint, or explicitly disable channel bindings on the second authentication pass in order to allow the MITM. (Or, throwing spaghetti, maybe even have the primary server communicate the backend cert so you can verify it and use it in the binding?) All that is a metric ton more work and analysis, though. --Jacob [1] https://www.postgresql.org/message-id/9129a012-0415-947e-a68e-59d423071525%40timescale.com ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: SCRAM pass-through authentication for postgres_fdw 2024-12-04 18:44 SCRAM pass-through authentication for postgres_fdw Matheus Alcantara <[email protected]> 2024-12-04 22:11 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 2024-12-04 23:05 ` Re: SCRAM pass-through authentication for postgres_fdw Jelte Fennema-Nio <[email protected]> 2024-12-04 23:39 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> @ 2024-12-04 23:49 ` Jacob Champion <[email protected]> 1 sibling, 0 replies; 8+ messages in thread From: Jacob Champion @ 2024-12-04 23:49 UTC (permalink / raw) To: Jelte Fennema-Nio <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Dec 4, 2024 at 3:39 PM Jacob Champion <[email protected]> wrote: > actual pass-through SCRAM (This was probably not a helpful way to describe what I'm talking about; the method here in the thread could be considered "actual pass-through SCRAM" as well. Proxied SCRAM, maybe?) --Jacob ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: SCRAM pass-through authentication for postgres_fdw 2024-12-04 18:44 SCRAM pass-through authentication for postgres_fdw Matheus Alcantara <[email protected]> 2024-12-04 22:11 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 2024-12-04 23:05 ` Re: SCRAM pass-through authentication for postgres_fdw Jelte Fennema-Nio <[email protected]> 2024-12-04 23:39 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> @ 2024-12-11 19:04 ` Matheus Alcantara <[email protected]> 2024-12-12 16:44 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 1 sibling, 1 reply; 8+ messages in thread From: Matheus Alcantara @ 2024-12-11 19:04 UTC (permalink / raw) To: Jacob Champion <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]> Em qua., 4 de dez. de 2024 às 20:39, Jacob Champion <[email protected]> escreveu: > > On Wed, Dec 4, 2024 at 3:05 PM Jelte Fennema-Nio <[email protected]> wrote: > > I only see advantages over the > > alternative, which is copying the plaintext password around. In case > > of compromise of the server, only the salt+verifier has to be rotated, > > not the actual user password. > > Sure, I'm not saying it's worse than plaintext. But a third > alternative might be actual pass-through SCRAM [1], where either you > expect the two servers to share a certificate fingerprint, or > explicitly disable channel bindings on the second authentication pass > in order to allow the MITM. (Or, throwing spaghetti, maybe even have > the primary server communicate the backend cert so you can verify it > and use it in the binding?) I'm not understanding how these options would work for this scenario. I understand your concern about making the users copying the SCRAM verifiers around but I don't understand how this third alternative fix this issue. Would it be something similar to what you've implemented on [1]? The approach on this patch is that when the backend open the connection with the foreign server, it will use the ClientKey stored from the first client connection with the backend to build the final client message that will be sent to the foreign server, which was created/validated based on verifiers of the first backend. I can't see how the foreign server can validate the client proof without having the identical verifiers with the first backend. I tested a scenario where the client open a connection with the backend using channel binding and the backend open a connection with the foreign server also using channel binding and everything seems to works fine. I don't know if I missing something here, but here is how I've tested this: - Configure build system to use openssl meson setup build -Dssl=openssl ... - Start two Postgresql servers - Configure to use ssl on both servers ssl = on ssl_cert_file = '/path/to/server.crt' ssl_key_file = '/path/to/server.key' - Changed pg_hba to use ssl on both servers hostssl all all 127.0.0.1/32 scram-sha-256 hostssl all all ::1/128 scram-sha-256 - Performed all foreign server setup (CREATE SERVER ...) - Connect into the backend using channel_binding=require psql "host=127.0.0.1 dbname=local sslmode=require channel_binding=require" - Execute a query on fdw server I've also put a debug message before strcmp(channel_binding, b64_message) [2] to ensure that channel binding is being used on both servers and I got the log message on both servers logs. Sorry if I misunderstood your message, I probably missed something here. [1] https://www.postgresql.org/message-id/9129a012-0415-947e-a68e-59d423071525%40timescale.com [2] src/backend/libpq/auth-scram.c#read_client_final_message -- Matheus Alcantara EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: SCRAM pass-through authentication for postgres_fdw 2024-12-04 18:44 SCRAM pass-through authentication for postgres_fdw Matheus Alcantara <[email protected]> 2024-12-04 22:11 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 2024-12-04 23:05 ` Re: SCRAM pass-through authentication for postgres_fdw Jelte Fennema-Nio <[email protected]> 2024-12-04 23:39 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 2024-12-11 19:04 ` Re: SCRAM pass-through authentication for postgres_fdw Matheus Alcantara <[email protected]> @ 2024-12-12 16:44 ` Jacob Champion <[email protected]> 0 siblings, 0 replies; 8+ messages in thread From: Jacob Champion @ 2024-12-12 16:44 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Dec 11, 2024 at 11:04 AM Matheus Alcantara <[email protected]> wrote: > Em qua., 4 de dez. de 2024 às 20:39, Jacob Champion > <[email protected]> escreveu: > > Sure, I'm not saying it's worse than plaintext. But a third > > alternative might be actual pass-through SCRAM [1], where either you > > expect the two servers to share a certificate fingerprint, or > > explicitly disable channel bindings on the second authentication pass > > in order to allow the MITM. (Or, throwing spaghetti, maybe even have > > the primary server communicate the backend cert so you can verify it > > and use it in the binding?) > > I'm not understanding how these options would work for this scenario. > I understand your concern about making the users copying the SCRAM > verifiers around but I don't understand how this third alternative fix > this issue. Would it be something similar to what you've implemented on > [1]? Yeah, I was speaking in reference to my LDAP/SCRAM patchset from a while back. (I'm just going to call that approach "proxied SCRAM" for now.) > The approach on this patch is that when the backend open the > connection with the foreign server, it will use the ClientKey stored > from the first client connection with the backend to build the final > client message that will be sent to the foreign server, which was > created/validated based on verifiers of the first backend. I can't see > how the foreign server can validate the client proof without having > the identical verifiers with the first backend. Correct. The only way this strategy will work is if the verifiers are the same. (Proxied SCRAM allows for different verifiers -- with different salts and/or iterations -- with the same password.) I do like that the action "copy the verifier" is a pretty clear signal that you want the servers to be able to MITM each other. It's less attack surface than having the two servers share a certificate, for sure, and less work than communicating a new binding. Only users that have opted into that are "vulnerable". > I tested a scenario where the client open a connection with the > backend using channel binding and the backend open a connection with > the foreign server also using channel binding and everything seems to > works fine. I don't know if I missing something here, but here is how > I've tested this: All that looks good. Sorry, I hadn't intended to derail your particular proposal with that -- the channel binding problem only shows up with my proxied SCRAM, because the client has to decide which tls-server-end-point to trust and put into the binding. (It's important that your patchset works with channel bindings too, of course, but I don't expect that to be a problem. It shouldn't matter to this approach.) --Jacob ^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2024-12-12 16:44 UTC | newest] Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2021-02-17 00:02 [PATCH 4/5] WIP rework tracking of expressions Tomas Vondra <[email protected]> 2024-12-04 18:44 SCRAM pass-through authentication for postgres_fdw Matheus Alcantara <[email protected]> 2024-12-04 22:11 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 2024-12-04 23:05 ` Re: SCRAM pass-through authentication for postgres_fdw Jelte Fennema-Nio <[email protected]> 2024-12-04 23:39 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 2024-12-04 23:49 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]> 2024-12-11 19:04 ` Re: SCRAM pass-through authentication for postgres_fdw Matheus Alcantara <[email protected]> 2024-12-12 16:44 ` Re: SCRAM pass-through authentication for postgres_fdw Jacob Champion <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox