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

Fix auth's wildcard Match function #4828

Merged
merged 2 commits into from
Dec 19, 2022
Merged

Fix auth's wildcard Match function #4828

merged 2 commits into from
Dec 19, 2022

Conversation

nopcoder
Copy link
Contributor

@nopcoder nopcoder commented Dec 16, 2022

Address performance and malicious usage.
Fix #1996

Added unit test code from MinIO for testing the functionality.

Used the following modified benchmark code supplied by @arielshaqed to test code for malicious usage:

package wildcard

import (
	"strings"
	"testing"
)

func BenchmarkMatch(b *testing.B) {
	const pattern = "a*b*c*z"
	str := strings.Repeat("a", 1000) + strings.Repeat("b", 1000) + strings.Repeat("c", 1000)
	for i := 0; i < b.N; i++ {
		if Match(pattern, str) {
			b.Errorf("Expecteded '%s' NOT to match '%s'", str, pattern)
		}
	}
}

func BenchmarkMatchDuh(b *testing.B) {
	const pattern = "a*a*a*z"
	str := strings.Repeat("a", 2000)
	for i := 0; i < b.N; i++ {
		if Match(pattern, str) {
			b.Errorf("Expecteded '%s' NOT to match '%s'", str, pattern)
		}
	}
}

Test results of current implementation:

BenchmarkMatch-10                      1        4474831292 ns/op
BenchmarkMatchDuh-10                   1        11981327834 ns/op

Test results of the new implementation:

BenchmarkMatch-10                 176950              6804 ns/op
BenchmarkMatchDuh-10              264994              4511 ns/op

Address performance and malicious usage.
Fix #1996
@nopcoder nopcoder added bug Something isn't working area/auth IAM, authorization, authentication, audit, AAA, and integrations with all those include-changelog PR description should be included in next release changelog labels Dec 16, 2022
@nopcoder nopcoder self-assigned this Dec 16, 2022
@nopcoder nopcoder requested a review from arielshaqed December 16, 2022 16:24
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Nice one, thanks!

I had some difficulty understanding the algorithm at first (it doesn't backtrack or consider more than one possibility at any point!). For future readers and especially future me here are my notes on how it works. So very safe to skip this!

Call a "fixword" a maximal portion of the pattern consisting only of regular characters and ?s. So a fixword has to begin after * or at the beginning of the string, and it has to end before * or at the end of the string.

Each fixword matches a fixed length of string. Now a pattern is a list of fixwords separated by *s. Consider a fixword that is not preceded by a *; that's an easy match to find because it can only be at one place. Consider a fixword that is preceded by a *; if it matches at multiple places then it is always safe to match it at the first possible location: either the pattern ends after that fixword in which case there's only one possible location, or the pattern continues with *, in which case that * can "expand" to pick up all characters and the next match of the fixword.

Either way, it's safe, so everything works.

@nopcoder
Copy link
Contributor Author

@arielshaqed thank you, credit to the Go team - I'll add the above explanation to the code so other will enjoy it.

@nopcoder nopcoder enabled auto-merge (squash) December 19, 2022 08:33
@nopcoder nopcoder merged commit de15c39 into master Dec 19, 2022
@nopcoder nopcoder deleted the fix/auth-match branch December 19, 2022 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/auth IAM, authorization, authentication, audit, AAA, and integrations with all those bug Something isn't working include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Malicious user can DoS lakeFS CPU for some IAM patterns
2 participants