Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added in basic auth and no-negotiate options #2204

Closed
wants to merge 3 commits into from

Conversation

btoneill
Copy link

Two big options for GSSAPI were not in the module, added them in.

@btoneill btoneill requested a review from a team as a code owner October 14, 2021 20:01
@puppet-community-rangefinder
Copy link

apache::vhost is a type

that may have no external impact to Forge modules.

This module is declared in 172 of 578 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

@david22swan
Copy link
Member

@btoneill This change look's interesting, but in order to get it merged we would need some additional documentation and test coverage.

@raybellis
Copy link

Please do merge this - some of the other GSSAPI module's options would also be useful (e.g. `GssapiUseSessions')

@github-actions
Copy link

This PR has been marked as stale because it has been open for a while and has had no recent activity. If this PR is still important to you please drop a comment below and we will add this to our backlog to complete. Otherwise, it will be closed in 7 days.

@github-actions github-actions bot added the stale label Apr 26, 2022
@raybellis
Copy link

We still need this, and other GSSAPI options. At the moment I'm having to include the following as custom configs on our servers:

  $custom_gssapi = '
    GssapiBasicAuth On
    GssapiUseSessions On
    Session On
    SessionCookieName ovirt_gssapi_session path=/;httponly;secure
    AuthDBMGroupFile /etc/http.groups
    AuthzDBMType default
    BrowserMatch Windows gssapi-no-negotiate
'

@github-actions github-actions bot removed the stale label Apr 27, 2022
@LukasAud
Copy link
Contributor

Hi @raybellis, sorry for the delay in feedback. We will be moving this back to active PRs and, hopefully, someone on our team can look at this soon.

Meanwhile, as per @david22swan request, could you give us a bit more context on this change? What behaviour are these options responsible for? Are these common options or something more on the specialised area of apache? Any information you can provide will help us make a decision faster. Thank you for your patience.

@raybellis
Copy link

raybellis commented May 5, 2022

These options (except for the two Auth* ones) are likely to be wanted by anyone using Kerberos via Basic Authentication, as opposed to requiring that the client browser pass a Kerberos ticket via GSSAPI.

Copy link
Member

@david22swan david22swan left a comment

Choose a reason for hiding this comment

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

@btoneill Hey, sorry for the wait on review.

Anyway while the changes look good to me, aside from one small error I have noted, we would need some additional tests added in order to cover the new variables, the relevant test file being linked below:

In addition while reviewing other apache PRs I found 3 others adding similar functionality to this one, one of which adds basicauth and has been merged in, i.e.:

Another that adds negotiate_once and has not yet been merged:

And a third that adds another 3 variables:

This is just a head's up so that you know to rebase your work and are aware that it may be fulfilled by another PR, depending on if it get's in a merge ready state before your own.

@@ -11,6 +14,12 @@
<% if $localname { -%>
GssapiLocalName <%= $localname %>
<% } -%>
<% if $negotiateonce { -%>
GssapiNegotiateOnce <%= $basicauth %>
Copy link
Member

Choose a reason for hiding this comment

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

Small error, but should this not be:

GssapiNegotiateOnce <%= $negotiateonce %>

@david22swan
Copy link
Member

@btoneill Hey, sorry but gonna close this PR as another has been merged in that adds the same functionality:
#2214
Thanks for putting in the work though and I hope to see more from you in the future.

@david22swan david22swan closed this Jun 8, 2022
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.

None yet

5 participants