Skip to content

Conversation

@akerouanton
Copy link
Member

Related to:

- What I did

Update the "Deprecated Engine Features" doc page to warn about daemon's api-cors-header deprecation.

@codecov-commenter
Copy link

codecov-commenter commented May 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.31%. Comparing base (57a1180) to head (9d9bb19).
Report is 16 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5085      +/-   ##
==========================================
- Coverage   61.32%   61.31%   -0.01%     
==========================================
  Files         298      295       -3     
  Lines       20706    20701       -5     
==========================================
- Hits        12698    12693       -5     
+ Misses       7106     7105       -1     
- Partials      902      903       +1     

Comment on lines 121 to 122
Daemon's `api-cors-header` flag is deemed insecure as it could be enabled
without any Authz plugin enabled beforehand.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if we can make this description more generic; Authz plugins are one way to do this, but are also a bit of a corner-case use (there may be many other approaches). The authz plugin feature also isn't very actively maintained, so somewhat trying to avoid it coming across as a recommendation.

cc @dvdksn in case you have good suggestions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we just pretend Authz was short for authorization?

Suggested change
Daemon's `api-cors-header` flag is deemed insecure as it could be enabled
without any Authz plugin enabled beforehand.
Daemon's `api-cors-header` flag is deemed insecure as it could be enabled
without any authorization plugin enabled beforehand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's another try from me:

Suggested change
Daemon's `api-cors-header` flag is deemed insecure as it could be enabled
without any Authz plugin enabled beforehand.
The `api-cors-header` configuration option for the Docker daemon is insecure,
and is therefore deprecated and scheduled for removal.
Incorrectly setting this option could leave a window of opportunity
for unauthenticated cross-origin requests to be accepted by the daemon.
Starting in Docker Engine v27.0, this flag can still be set,
but it has no effect unless the environment variable
`DOCKERD_DEPRECATED_CORS_HEADER` is also set to a non-empty value.
This flag will be removed altogether in v28.0.
This is a breaking change for authorization plugins and other programs
that depend on this option for accessing the Docker API from a browser.
If you need to access the API through a browser, use a reverse proxy.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @dvdksn!

@akerouanton akerouanton force-pushed the deprecate-api-cors-header branch from d2a59da to 9d9bb19 Compare May 28, 2024 10:10
@akerouanton akerouanton requested a review from thaJeztah May 28, 2024 10:22
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
Copy link
Member

I'll bring this one in; one more thing we may need to consider is to add some notes on https://github.com/docker/cli/blob/6a4d38c7f2fe17351f78ff9dfe7ec2ee9b15f1df/docs/reference/dockerd.md

The dockerd docs are not (yet) generated from Cobra, so that means we won't have a table with options / annotations that flags are deprecated. And we also have the option documented for the daemon.json config.

@dvdksn if you have any good ideas how to update those docs; let me know. I don't expect it to be a popular option, but .. well .. someone, somewhere will likely use it.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants