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

[PXP-8941] [PXP-8859] Feat/update go 1.17 #25

Merged
merged 9 commits into from
Nov 16, 2021
Merged

Conversation

m0nhawk
Copy link
Contributor

@m0nhawk m0nhawk commented Oct 8, 2021

Jira Ticket: PXP-8941
Jira Ticket: PXP-8859

New Features

Breaking Changes

Bug Fixes

  • Fix for x509: certificate signed by unknown authority

Improvements

  • Proper verification of JWT token
  • Update Go 1.17 (requirements to properly verify JWT token
  • Build flags to make GitCommit and GitVersion available for the code

Dependency updates

Deployment changes

PS: This is follow-up on this one: #24

@m0nhawk m0nhawk requested review from mfshao and themarcelor and removed request for themarcelor October 8, 2021 14:27
return nil, fmt.Errorf("Unexpected signing method: %v", token.Header["alg"])
}
func getEmailFromToken(accessTokenVal string) (string, error) {
jwksURL := "http://fence-service/.well-known/openid-configuration"
Copy link
Contributor

Choose a reason for hiding this comment

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

should http://fence-service be a config default instead of hardcoded?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does makes sense... But I'm not sure we really need this. Is there a scenario where the fence pod is not running under fence-service?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we run the service locally, or in an ecosystem setup like we used to have for NIAID NDE (sub-commons microservices talking to a central fence). But maybe it's too YAGNI 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For running locally, it should be OK (I’m the context of compose-services, for example).

I’m case of NIAID NDE, I’m not sure on the implications… it’s really depends on who issues the access token for the user and where it runs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@paulineribeyre I would say — let's merge this for now, and keep an eye on if this will be needed by anyone.

handlers/sower.go Show resolved Hide resolved
handlers/sower.go Outdated Show resolved Hide resolved
handlers/jobs.go Show resolved Hide resolved
paulineribeyre
paulineribeyre previously approved these changes Oct 12, 2021
@m0nhawk m0nhawk changed the title Feat/update go 1.17 [PXP-8941] Feat/update go 1.17 Oct 27, 2021
@m0nhawk m0nhawk changed the title [PXP-8941] Feat/update go 1.17 [PXP-8941 PXP-8859] Feat/update go 1.17 Oct 27, 2021
@m0nhawk m0nhawk changed the title [PXP-8941 PXP-8859] Feat/update go 1.17 [PXP-8941] [PXP-8859] Feat/update go 1.17 Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants