You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/06/17 22:56:02 UTC
[GitHub] [trafficserver] traeak opened a new pull request, #8918: uri_signing/parse: check for null iss when kid == NULL
traeak opened a new pull request, #8918:
URL: https://github.com/apache/trafficserver/pull/8918
In the uri_signing plugin add a check for non null iss in the case where kid == NULL. Previous PR added the check for then kid != NULL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8918: uri_signing/parse: check for null iss when kid == NULL
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8918:
URL: https://github.com/apache/trafficserver/pull/8918#discussion_r903710432
##########
plugins/experimental/uri_signing/parse.c:
##########
@@ -222,6 +222,10 @@ validate_jws(cjose_jws_t *jws, struct config *cfg, const char *uri, size_t uri_c
}
TimerDebug("checking crypto signature for jwt");
} else {
+ if (!jwt->iss) {
Review Comment:
I think if you chery-pick [this](https://github.com/apache/trafficserver/pull/8901/) change then this will not core dump at least because of the missing `iss`, it will instead fail the validation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8918: uri_signing/parse: check for null iss when kid == NULL
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8918:
URL: https://github.com/apache/trafficserver/pull/8918#discussion_r901522006
##########
plugins/experimental/uri_signing/parse.c:
##########
@@ -222,6 +222,10 @@ validate_jws(cjose_jws_t *jws, struct config *cfg, const char *uri, size_t uri_c
}
TimerDebug("checking crypto signature for jwt");
} else {
+ if (!jwt->iss) {
Review Comment:
maybe I am missing something:
I guess it does not harm but, If the `jwt_validate` [up](https://github.com/apache/trafficserver/blob/8ca74ee27cc14f27a1480a88fc453fbabc954e4e/plugins/experimental/uri_signing/parse.c#L195) above fails because `iss` is null then we should never get this far. Previous [PR](https://github.com/apache/trafficserver/pull/8901/) added a validation for this field in which case it should prevent us getting this far.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak commented on a diff in pull request #8918: uri_signing/parse: check for null iss when kid == NULL
Posted by GitBox <gi...@apache.org>.
traeak commented on code in PR #8918:
URL: https://github.com/apache/trafficserver/pull/8918#discussion_r902530399
##########
plugins/experimental/uri_signing/parse.c:
##########
@@ -222,6 +222,10 @@ validate_jws(cjose_jws_t *jws, struct config *cfg, const char *uri, size_t uri_c
}
TimerDebug("checking crypto signature for jwt");
} else {
+ if (!jwt->iss) {
Review Comment:
You could be correct. This path was core dumping with the autest after applying some internal patches. I can dig a bit to see if I can move the check up higher.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak commented on pull request #8918: uri_signing/parse: check for null iss when kid == NULL
Posted by GitBox <gi...@apache.org>.
traeak commented on PR #8918:
URL: https://github.com/apache/trafficserver/pull/8918#issuecomment-1164685063
The current logic should make it so this is never hit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on a diff in pull request #8918: uri_signing/parse: check for null iss when kid == NULL
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on code in PR #8918:
URL: https://github.com/apache/trafficserver/pull/8918#discussion_r901522006
##########
plugins/experimental/uri_signing/parse.c:
##########
@@ -222,6 +222,10 @@ validate_jws(cjose_jws_t *jws, struct config *cfg, const char *uri, size_t uri_c
}
TimerDebug("checking crypto signature for jwt");
} else {
+ if (!jwt->iss) {
Review Comment:
Not an issue but, If the `jwt_validate` [up](https://github.com/apache/trafficserver/blob/8ca74ee27cc14f27a1480a88fc453fbabc954e4e/plugins/experimental/uri_signing/parse.c#L195) above fails because `iss` is null then we should never get this far. Previous [PR](https://github.com/apache/trafficserver/pull/8901/) added a validation for this field in which case it should prevent us getting this far.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak commented on a diff in pull request #8918: uri_signing/parse: check for null iss when kid == NULL
Posted by GitBox <gi...@apache.org>.
traeak commented on code in PR #8918:
URL: https://github.com/apache/trafficserver/pull/8918#discussion_r905292307
##########
plugins/experimental/uri_signing/parse.c:
##########
@@ -222,6 +222,10 @@ validate_jws(cjose_jws_t *jws, struct config *cfg, const char *uri, size_t uri_c
}
TimerDebug("checking crypto signature for jwt");
} else {
+ if (!jwt->iss) {
Review Comment:
The jwt_validate should take care of this in the normal code. I'll need to further review our internal patch.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak closed pull request #8918: uri_signing/parse: check for null iss when kid == NULL
Posted by GitBox <gi...@apache.org>.
traeak closed pull request #8918: uri_signing/parse: check for null iss when kid == NULL
URL: https://github.com/apache/trafficserver/pull/8918
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] zwoop commented on pull request #8918: uri_signing/parse: check for null iss when kid == NULL
Posted by GitBox <gi...@apache.org>.
zwoop commented on PR #8918:
URL: https://github.com/apache/trafficserver/pull/8918#issuecomment-1176519945
Remember to remove projects and milestone when closing without merging.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] brbzull0 commented on pull request #8918: uri_signing/parse: check for null iss when kid == NULL
Posted by GitBox <gi...@apache.org>.
brbzull0 commented on PR #8918:
URL: https://github.com/apache/trafficserver/pull/8918#issuecomment-1160290036
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] traeak commented on pull request #8918: uri_signing/parse: check for null iss when kid == NULL
Posted by GitBox <gi...@apache.org>.
traeak commented on PR #8918:
URL: https://github.com/apache/trafficserver/pull/8918#issuecomment-1159468376
[approve ci autest]
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org