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

Add Redis Sentinel Authentication Support #19213

Merged
merged 10 commits into from
Mar 30, 2022
Merged

Conversation

jsievenpiper
Copy link
Contributor

👋🏾 Hey there!

I was looking to configure my gitea cache to use my existing redis sentinel setup, but found that I wasn't able to supply any authentication parameters to it. So this PR brings support to do that, along with some light extraction of a couple of bits into some separate functions for easier testing (and of course, some tests as well).

I looked at other libraries supporting similar RedisUri-style connection strings (e.g. Lettuce), but it looks like this type of configuration is beyond what would typically be done in a connection string. Since gitea doesn't have configuration options for manually specifying all this redis connection detail, I went ahead and just chose straightforward names for these new parameters.

I think there are two main concerns with this approach:

  • the implementation details are a bit confusing -- to be fair, it can be confusing in general to consider the ACL requirements in a sentinel setup just in general, but having two sets of credentials in a single connection string is definitely more complicated.
  • putting credentials outside the standard URI location exposes them to being leaked more easily... granted, I think this is not hugely concerning since a practical installation would have this coming from some sort of secrets management system -- more ideally these configuration options would be directly exposed in gitea's configuration, but it looks like the spirit of the config file was to be really abstract for this section, so not sure there's much to be done there without refactoring and making a breaking change in the config file schema.

Looking forward to your feedback!

modules/nosql/manager_redis.go Outdated Show resolved Hide resolved
modules/nosql/manager_redis.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 25, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #19213 (b475df1) into main (49c5fc5) will decrease coverage by 0.03%.
The diff coverage is 35.39%.

@@            Coverage Diff             @@
##             main   #19213      +/-   ##
==========================================
- Coverage   46.55%   46.52%   -0.04%     
==========================================
  Files         856      857       +1     
  Lines      123018   123235     +217     
==========================================
+ Hits        57277    57341      +64     
- Misses      58814    58951     +137     
- Partials     6927     6943      +16     
Impacted Files Coverage Δ
cmd/admin.go 0.00% <0.00%> (ø)
cmd/hook.go 7.11% <0.00%> (ø)
cmd/serv.go 2.39% <0.00%> (ø)
models/asymkey/ssh_key_deploy.go 55.55% <ø> (+1.13%) ⬆️
models/helper_environment.go 100.00% <ø> (ø)
modules/context/permission.go 25.39% <0.00%> (ø)
modules/doctor/checkOldArchives.go 22.85% <0.00%> (ø)
modules/doctor/fix16961.go 35.06% <0.00%> (ø)
modules/doctor/mergebase.go 10.25% <0.00%> (ø)
modules/doctor/misc.go 21.55% <0.00%> (ø)
... and 179 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d2c1658...b475df1. Read the comment docs.

@jsievenpiper
Copy link
Contributor Author

@zeripath thanks for the quick review! Updated the code style 👍🏾

@zeripath
Copy link
Contributor

Hmm...

Do you know if the "FailoverClient" uses the username/password options that we already provide? If not could we just copy them over to the sentinelusername/password instead of creating more options?

If not I guess we have to add this.

@jsievenpiper
Copy link
Contributor Author

I did some of the sentinel implementation in go-redis so I can answer this!

It doesn't -- sentinels are a separate server than the main redis instances, and can (and often are) authenticated separately, especially in a Redis 6-7 world where ACL auth allows you to restrict scope on a user-level basis. i.e. I only get support for looking for the main redis instance via my sentinel credentials, but I can then use much more powerful read/write credentials against the main redis instance. That's why UniversalOptions and FailoverOptions have separate fields for each.

modules/nosql/manager_redis.go Outdated Show resolved Hide resolved
modules/nosql/manager_redis.go Outdated Show resolved Hide resolved
modules/nosql/manager_redis.go Outdated Show resolved Hide resolved
modules/nosql/manager_redis.go Show resolved Hide resolved
modules/nosql/manager_redis.go Outdated Show resolved Hide resolved
@lunny lunny added the type/enhancement An improvement of existing functionality label Mar 26, 2022
@lunny lunny added this to the 1.17.0 milestone Mar 26, 2022
@zeripath
Copy link
Contributor

It doesn't -- sentinels are a separate server than the main redis instances, and can (and often are) authenticated separately, especially in a Redis 6-7 world where ACL auth allows you to restrict scope on a user-level basis. i.e. I only get support for looking for the main redis instance via my sentinel credentials, but I can then use much more powerful read/write credentials against the main redis instance. That's why UniversalOptions and FailoverOptions have separate fields for each.

So what I'm asking is if the opts.Failover() don't pass the opts.Username/opts.Password or use them ... Then instead of creating new query options in the connection string we can just reuse the current username and password parts of the uri and on use of sentinel we just copy the username password to the appropriate parts of the options.

If not fine, it's fine to create a new set of query options.

@jsievenpiper
Copy link
Contributor Author

So what I'm asking is if the opts.Failover() don't pass the opts.Username/opts.Password or use them ... Then instead of creating new query options in the connection string we can just reuse the current username and password parts of the uri and on use of sentinel we just copy the username password to the appropriate parts of the options.

I believe I understand you, but my response should still be valid -- you need both sets of credentials for sentinel. The FailoverClient uses a set of credentials for authentication against the sentinel (what this PR adds) and a separate set of credentials once the sentinels have designated a redis master instance, in which the client will then connect with the other (existing) set of credentials.

So FailoverOptions provides both sets of credentials because it's a two-phase process with two different authentication credential pairs.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 28, 2022
Copy link
Member

@6543 6543 left a comment

Choose a reason for hiding this comment

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

#19213 (comment)

else ack

@jsievenpiper
Copy link
Contributor Author

@6543

else ack

Roger that, new log line tossed in -- whipped up some tests there as well

@6543
Copy link
Member

6543 commented Mar 30, 2022

please no force push - I have to review all again

@6543
Copy link
Member

6543 commented Mar 30, 2022

we squash-merge so you can just merge master into your branch to resolve conflicts

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 30, 2022
@6543 6543 merged commit a2c20a6 into go-gitea:main Mar 30, 2022
@6543
Copy link
Member

6543 commented Mar 30, 2022

nice work - and thanks for the tests - appreciate them :)

@jsievenpiper jsievenpiper deleted the sentinel branch March 30, 2022 20:19
@wxiaoguang
Copy link
Contributor

Very good refactoring 👍

zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 31, 2022
* giteaofficial/main:
  Update reserved usernames list (go-gitea#18438)
  Configure OpenSSH log level via Environment in Docker (go-gitea#19274)
  Use a more general (and faster) method to sanitize URLs with credentials (go-gitea#19239)
  [skip ci] Updated translations via Crowdin
  fix link to package registry docs (go-gitea#19268)
  Add Redis Sentinel Authentication Support (go-gitea#19213)
@jsievenpiper
Copy link
Contributor Author

Thanks everyone! Appreciate the active community here 👍🏾

@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
zeripath added a commit that referenced this pull request Sep 4, 2022
) (#21053)

Backport #20967

Currently, it's impossible to connect to self-signed TLS encrypted redis instances. The problem lies in inproper error handling, when building redis tls options - only invalid booleans are allowed to be used in `tlsConfig` builder. The problem is, when `strconv.ParseBool(...)` returns error, it always defaults to false - meaning it's impossible to set `tlsOptions.InsecureSkipVerify` to true.

Fixes #19213

Co-authored-by: Igor Rzegocki <ajgon@users.noreply.github.com>
tyroneyeh added a commit to tyroneyeh/gitea that referenced this pull request Sep 7, 2022
commit 32eef4a
Author: Lunny Xiao <xiaolunwen@gmail.com>
Date:   Wed Sep 7 05:32:20 2022 +0800

    Add changelog for v1.17.2 (go-gitea#21089)

    Co-authored-by: John Olheiser <john+github@jolheiser.com>
    Co-authored-by: 6543 <6543@obermui.de>
    Co-authored-by: delvh <dev.lh@web.de>
    Co-authored-by: techknowlogick <techknowlogick@gitea.io>

commit 449b39e
Author: Tyrone Yeh <tyrone_yeh@draytek.com>
Date:   Tue Sep 6 16:42:05 2022 +0800

    Fix sub folder in repository missing add file dropdown (go-gitea#21069) (go-gitea#21083)

    Backport go-gitea#21069

    In repository sub folder missing add file dropdown menu, Probably broken since go-gitea#20602

commit 06f968d
Author: zeripath <art27@cantab.net>
Date:   Tue Sep 6 07:54:47 2022 +0100

    Fix hard-coded timeout and error panic in API archive download endpoint (go-gitea#20925) (go-gitea#21051)

    Backport go-gitea#20925

    This commit updates the `GET /api/v1/repos/{owner}/{repo}/archive/{archive}`
    endpoint which prior to this PR had a couple of issues.

    1. The endpoint had a hard-coded 20s timeout for the archiver to complete after
       which a 500 (Internal Server Error) was returned to client. For a scripted
       API client there was no clear way of telling that the operation timed out and
       that it should retry.

    2. Whenever the timeout _did occur_, the code used to panic. This was caused by
       the API endpoint "delegating" to the same call path as the web, which uses a
       slightly different way of reporting errors (HTML rather than JSON for
       example).

       More specifically, `api/v1/repo/file.go#GetArchive` just called through to
       `web/repo/repo.go#Download`, which expects the `Context` to have a `Render`
       field set, but which is `nil` for API calls. Hence, a `nil` pointer error.

    The code addresses (1) by dropping the hard-coded timeout. Instead, any
    timeout/cancelation on the incoming `Context` is used.

    The code addresses (2) by updating the API endpoint to use a separate call path
    for the API-triggered archive download. This avoids producing HTML-errors on
    errors (it now produces JSON errors).

    Signed-off-by: Peter Gardfjäll <peter.gardfjall.work@gmail.com>

    Signed-off-by: Peter Gardfjäll <peter.gardfjall.work@gmail.com>
    Signed-off-by: Andrew Thornton <art27@cantab.net>
    Co-authored-by: Peter Gardfjäll <peter.gardfjall.work@gmail.com>
    Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

commit 084797b
Author: Lunny Xiao <xiaolunwen@gmail.com>
Date:   Tue Sep 6 06:48:57 2022 +0800

    Fix delete user missed some comments (go-gitea#21067) (go-gitea#21068)

commit 7888a55
Author: zeripath <art27@cantab.net>
Date:   Sun Sep 4 17:17:48 2022 +0100

    Delete unreferenced packages when deleting a package version (go-gitea#20977) (go-gitea#21060)

    Backport go-gitea#20977

    Delete a package if its last version got deleted. Otherwise removing the owner works only after the clean up job ran.

    Fix go-gitea#20969

    Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

commit ea416d7
Author: zeripath <art27@cantab.net>
Date:   Sun Sep 4 17:17:35 2022 +0100

    Redirect if user does not exist on admin pages (go-gitea#20981) (go-gitea#21059)

    Backport go-gitea#20981

    When on /admin/users/ endpoints if the user is no longer in the DB,
    redirect instead of causing a http 500.

    Co-authored-by: KN4CK3R <admin@oldschoolhack.me>

commit 0db6add
Author: zeripath <art27@cantab.net>
Date:   Sun Sep 4 17:17:27 2022 +0100

    Set uploadpack.allowFilter etc on gitea serv to enable partial clones with ssh (go-gitea#20902) (go-gitea#21058)

    Backport go-gitea#20902

    When setting.Git.DisablePartialClone is set to false then the web server will add filter support to web http. It does this by using`-c` command arguments but this will not work on gitea serv as the upload-pack and receive-pack commands do not support this.

    Instead we move these options into the .gitconfig instead.

    Fix go-gitea#20400

    Signed-off-by: Andrew Thornton <art27@cantab.net>

    Signed-off-by: Andrew Thornton <art27@cantab.net>

commit 0ecbb71
Author: qwerty287 <80460567+qwerty287@users.noreply.github.com>
Date:   Sun Sep 4 17:12:37 2022 +0200

    Fix 500 on time in timeline API (go-gitea#21052) (go-gitea#21057)

    Backport go-gitea#21052

    Before converting a TrackedTime for the API we need to load its attributes - otherwise we get an NPE.

    Fix go-gitea#21041

commit ea38455
Author: Jason Song <i@wolfogre.com>
Date:   Sun Sep 4 23:12:01 2022 +0800

    Fill the specified ref in webhook test payload (go-gitea#20961) (go-gitea#21055)

    Backport go-gitea#20961

    The webhook payload should use the right ref when it‘s specified in the testing request.

    The compare URL should not be empty, a URL like `compare/A...A` seems useless in most cases but is helpful when testing.

commit 8fc80b3
Author: zeripath <art27@cantab.net>
Date:   Sun Sep 4 16:11:02 2022 +0100

    Add another index for Action table on postgres (go-gitea#21033) (go-gitea#21054)

    Backport go-gitea#21033

    In go-gitea#21031 we have discovered that on very big tables postgres will use a
    search involving the sort term in preference to the restrictive index.

    Therefore we add another index for postgres and update the original migration.

    Fix go-gitea#21031

    Signed-off-by: Andrew Thornton <art27@cantab.net>

commit 71aa64a
Author: zeripath <art27@cantab.net>
Date:   Sun Sep 4 14:59:20 2022 +0100

    fix broken insecureskipverify handling in rediss connection uris (go-gitea#20967) (go-gitea#21053)

    Backport go-gitea#20967

    Currently, it's impossible to connect to self-signed TLS encrypted redis instances. The problem lies in inproper error handling, when building redis tls options - only invalid booleans are allowed to be used in `tlsConfig` builder. The problem is, when `strconv.ParseBool(...)` returns error, it always defaults to false - meaning it's impossible to set `tlsOptions.InsecureSkipVerify` to true.

    Fixes go-gitea#19213

    Co-authored-by: Igor Rzegocki <ajgon@users.noreply.github.com>

commit 3aba72c
Author: zeripath <art27@cantab.net>
Date:   Sun Sep 4 14:41:21 2022 +0100

    Add more checks in migration code (go-gitea#21011) (go-gitea#21050)

    Backport go-gitea#21011

    When migrating add several more important sanity checks:

    * SHAs must be SHAs
    * Refs must be valid Refs
    * URLs must be reasonable

    Signed-off-by: Andrew Thornton <art27@cantab.net>

commit bd1412c
Author: José Carlos <joecarlhr@gmail.com>
Date:   Sat Sep 3 21:11:03 2022 +0200

    Add Dev, Peer and Optional dependencies to npm PackageMetadataVersion (go-gitea#21017) (go-gitea#21044)

    Backport go-gitea#21017

    Set DevDependencies, PeerDependencies & OptionalDependencies in npm package metadatas

    Fix go-gitea#21013

commit 3973ce3
Author: silverwind <me@silverwind.io>
Date:   Sat Sep 3 19:51:09 2022 +0200

    Improve arc-green code theme (go-gitea#21039) (go-gitea#21042)

    Backport go-gitea#21039

    - Increase contrasts overall
    - Add various missing theme classes
    - Ensure strings and constants are colored the same across languages

commit fbde31f
Author: Tyrone Yeh <tyrone_yeh@draytek.com>
Date:   Sat Sep 3 21:36:27 2022 +0800

    Add down key check has tribute container (go-gitea#21016) (go-gitea#21038)

    Backport go-gitea#21016

    Fixes an issue where users would not be able to select by pressing the down arrow when using @tag above a message

    Bug videos:

    https://user-images.githubusercontent.com/1255041/188095999-c4ccde18-e53b-4251-8a14-d90c4042d768.mp4
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants