Skip to content

Concurrent writes to a remote-signing catalog go out unsigned and fail 403Β #3896

Description

@paulcaron16k

Apache Iceberg version

main (development) β€” also reproduced on 0.12.0 and 0.11.1.

Please describe the bug 🐞

Table commits fail intermittently against a remote-signing catalog, because the request signer is unregistered while other threads are signing.

Roughly 3–4% of appends come back 403 AccessDenied under PyIceberg's own concurrent write path. PyIceberg's writer is concurrent by default, so no unusual usage is needed to reach this.

It surfaces as an opaque

PermissionError: Access Denied

raised out of s3fs, several frames from its cause, with nothing tying it to signing. Every unsigned request we caught on the wire was a PUT of a manifest (*-m0.avro) during commit β€” 5 of 5 and 3 of 3 across two runs β€” so what fails is the commit itself, not a stray read.

Environment: Lakekeeper 0.13.1 vending remote signing, MinIO, s3.path-style-access=true.


Root cause

pyiceberg/io/fsspec.py::_s3 installs the signer like this (lines 234–238 on main; the unregister/register pair is 237–238):

fs = S3FileSystem(**s3_fs_kwargs)

for event_name, event_function in register_events.items():
    fs.s3.meta.events.unregister(event_name, unique_id=1925)
    fs.s3.meta.events.register_last(event_name, event_function, unique_id=1925)

fsspec caches filesystem instances, so fs β€” and the botocore client and event emitter hanging off it β€” is shared by every thread. Between the unregister and the register_last no signer is installed, and _s3() has already set config_kwargs["signature_version"] = UNSIGNED, so botocore will not sign in its place. A request signed in that window goes out with no Authorization header at all, and the store answers 403 AccessDenied.

FsspecFileIO.get_fs caches per thread and every table gets its own FileIO, so a short workload performs over a hundred unregister/register cycles against that one shared emitter, each racing whatever writes are in flight.

The response code is the clue that took us longest to read correctly: it is always AccessDenied and never SignatureDoesNotMatch β€” which is what an unsigned request produces, not a mis-signed one.

Evidence

90 writes per run against a live stack, counting requests that reached the wire without an Authorization header (a before-send.s3 hook):

configuration append failures requests sent unsigned
as shipped 2 / 4 / 5 2 / 4 / 5
fsspec instance cache disabled (133 separate clients) 0 0
signer registered once, never unregistered (one shared client) 0 0
PYICEBERG_MAX_WORKERS=1 0 0

Unsigned-on-wire equals append failures exactly, run after run.

The third row is the one that matters: the failures stop while the client is still shared, which separates this from a general concurrency problem.

Suggested fix

One line, same place β€” drop the unregister:

for event_name, event_function in register_events.items():
    fs.s3.meta.events.register_last(event_name, event_function, unique_id=1925)

HierarchicalEmitter._register_section already returns early for a unique_id it holds:

if unique_id in self._unique_id_handlers:
    # We've already registered a handler using this unique_id
    # so we don't need to register it again.
    ...
    return

so re-registering an equivalent signer was already a no-op, and the unregister only opens the window. Every signer _s3() builds derives from the same properties, so which instance stays installed does not matter β€” only that one always is.

Reproduction

Fork and branch: paulcaron16k/iceberg-python @ bug/s3v4restsigner_unregisters_signer_mid_flight β€” one commit on top of current main, adding tests/io/test_fsspec_signer_registration.py. Seven tests, no credentials and no network.

The one that reads the library is test_s3_unregisters_the_signer_on_a_client_it_shares: two _s3() calls, fsspec hands back the same filesystem, and unregister is caught on both β€” the second while a perfectly good signer is installed. It fails once the unregister is dropped, which is what ties it to the fix rather than to incidental structure. The four existing signer tests in tests/io/test_fsspec.py pass before and after.

A second, independent defect in the same file

Reported here because it is four lines away, but it is not the cause of the above and can be fixed separately.

pyiceberg/io/fsspec.py:160 applies the signing service's headers with add_header, which appends:

for key, value in response_json["headers"].items():
    request.headers.add_header(key, ", ".join(value))

A signing service echoes back headers it was given as well as those it computed, so every echoed header ends up on the request twice. Instrumenting a real workload, 880 of 1615 sign calls ended with a duplicate of a header named in SignedHeaders β€” expect, x-amz-checksum-crc32, x-amz-sdk-checksum-algorithm on PutObject; if-match, range, x-amz-checksum-mode on HeadObject/GetObject.

In fairness: on MinIO this is latent. De-duplicating the headers left the failure count unchanged, and only the registration fix above stopped the 403s. But SigV4 combines repeated headers comma-separated, so a stricter implementation is entitled to reject them, and the signing service's header set is authoritative regardless.

It is invisible to the current tests because they assert on dict(request.headers), and dict() collapses repeated keys to one value β€” any regression test needs get_all.

Suggested fix, replace rather than append:

for key, value in response_json["headers"].items():
    if key in request.headers:
        del request.headers[key]
    request.headers.add_header(key, ", ".join(value))

Related, not proposed here

The fsspec cache key comes from s3_fs_kwargs, which does not include the signer's URI or endpoint β€” so two catalogs sharing an S3 endpoint and credentials but signing through different services share one filesystem. Today the last _s3() caller's signer wins; with the fix above the first one does. Neither is correct. The complete answer is for the signer configuration to participate in the filesystem's identity, but that changes which objects are cached, so we have left it as a separate decision rather than folding it in.

Willingness to contribute

  • I would be willing to contribute a fix for this bug with guidance from the Iceberg community

The branch above already carries the tests, and the one-line fix is validated against a live stack. Happy to open a PR for either or both defects β€” guidance welcome on whether you want them split, and on the cache-key question.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions