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

feat: HMAC SHA256 Authentication #795

Merged
merged 41 commits into from
Sep 19, 2023
Merged

Conversation

tswagger
Copy link
Contributor

@tswagger tswagger commented Aug 23, 2023

Adding HMAC-SHA256 as an authenticator. This method has a slight security advantage to a standard token authentication by signing the request with a shared secret.

Usage and coding mirrors closely the established Access Token Authentication classes and methods.

References:

@tswagger tswagger changed the title HMAC SHA256 feat: HMAC SHA256 Authentication Aug 23, 2023
@kenjis kenjis added GPG-Signing needed Pull requests that need GPG-Signing enhancement New feature or request labels Aug 24, 2023
@kenjis
Copy link
Member

kenjis commented Aug 24, 2023

Thank you for sending this PR!

You must GPG-sign your work, certifying that you either wrote the work or otherwise have the right to pass it on to an open-source project.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

@kenjis
Copy link
Member

kenjis commented Aug 24, 2023

And we don't use git merge to update PR branches.
Please use git rebase instead.
See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#updating-your-branch

@tswagger
Copy link
Contributor Author

Thank you for sending this PR!

You must GPG-sign your work, certifying that you either wrote the work or otherwise have the right to pass it on to an open-source project. See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/pull_request.md#signing

@kenjis My apologies, I thought I had that setup correctly. Since I obviously didn't, how do I retroactively sign what I have submitted?

@kenjis
Copy link
Member

kenjis commented Aug 24, 2023

See https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

@tswagger
Copy link
Contributor Author

@kenjis I have rebased and signed my code. I appreciate your assistance on that.

@kenjis kenjis removed the GPG-Signing needed Pull requests that need GPG-Signing label Aug 25, 2023
@kenjis
Copy link
Member

kenjis commented Aug 25, 2023

@tswagger Thank you!

@tswagger
Copy link
Contributor Author

tswagger commented Aug 25, 2023

I am not sure how you would like me to address the final failed check. I intentionally mirrored the Authorize Tokens classes. I could create a shared trait or abstract parent class, but some of the differences, while subtle, are major enough to make that challenging.

@kenjis
Copy link
Member

kenjis commented Aug 25, 2023

We will give it some consideration, so please leave it as it is.
In any case, this PR is too large to review easily.

Adding Trait or abstract classes could make the design worse.
We can also suppress errors by PHPCPD.

@tswagger
Copy link
Contributor Author

That was my thought. I will leave it in your hands. Please let me know if you need anything else from me.

docs/authentication.md Outdated Show resolved Hide resolved
docs/authentication.md Outdated Show resolved Hide resolved
docs/authentication.md Outdated Show resolved Hide resolved
docs/authentication.md Show resolved Hide resolved
docs/authentication.md Outdated Show resolved Hide resolved
docs/authentication.md Outdated Show resolved Hide resolved
docs/guides/api_hmac_keys.md Outdated Show resolved Hide resolved
docs/guides/api_hmac_keys.md Outdated Show resolved Hide resolved
@datamweb datamweb closed this Sep 5, 2023
@datamweb datamweb reopened this Sep 5, 2023
docs/authentication.md Outdated Show resolved Hide resolved
docs/authentication.md Outdated Show resolved Hide resolved
docs/guides/api_hmac_keys.md Outdated Show resolved Hide resolved
docs/guides/api_hmac_keys.md Outdated Show resolved Hide resolved
tswagger and others added 22 commits September 18, 2023 09:07
Co-authored-by: John Paul E. Balandan, CPA <[email protected]>
Co-authored-by: John Paul E. Balandan, CPA <[email protected]>
Signed-off-by: tswagger <[email protected]>
Signed-off-by: tswagger <[email protected]>
Added AuthToken config as a separate config for Token/HMAC auth from JWT
Updated test to reflect logging adjustment change.

Signed-off-by: tswagger <[email protected]>
Signed-off-by: tswagger <[email protected]>
Signed-off-by: tswagger <[email protected]>
Signed-off-by: tswagger <[email protected]>
Signed-off-by: tswagger <[email protected]>
Signed-off-by: tswagger <[email protected]>
@kenjis
Copy link
Member

kenjis commented Sep 18, 2023

Cannot reproduce the PHPStan errors.

(feature/HMAC $)$ vendor/bin/phpstan
Note: Using configuration file /Users/kenji/work/codeigniter/official/codeigniter-shield/phpstan.neon.dist.
 176/176 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


                                                                                                                        
 [OK] No errors 
 ------ -------------------------------------------------------------------- 
  Line   src/Models/UserModel.php                                            
 ------ -------------------------------------------------------------------- 
  215    Offset 'email' does not exist on array{username: string, status:    
         string, status_message: string, active: bool, last_active: string,  
         deleted_at: string}.                                                
  216    Cannot unset offset 'email' on array{username: string, status:      
         string, status_message: string, active: bool, last_active: string,  
         deleted_at: string}.                                                
  217    Offset 'password_hash' does not exist on array{username: string,    
         status: string, status_message: string, active: bool, last_active:  
         string, deleted_at: string}.                                        
  218    Cannot unset offset 'password_hash' on array{username: string,      
         status: string, status_message: string, active: bool, last_active:  
         string, deleted_at: string}.                                        
 ------ -------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------- 
  Line   tests/Unit/UserTest.php                                            
 ------ ------------------------------------------------------------------- 
         Ignored error pattern #^Cannot access property \$active on         
         array\|object\.$# in path                                          
         /home/runner/work/shield/shield/tests/Unit/UserTest.php was not    
         matched in reported errors.                                        
         Ignored error pattern #^Cannot access property \$password_hash on  
         array\|object\.$# in path                                          
         /home/runner/work/shield/shield/tests/Unit/UserTest.php was not    
         matched in reported errors.                                        
 ------ ------------------------------------------------------------------- 

Error:  [ERROR] Found 6 errors       

https://github.com/codeigniter4/shield/actions/runs/6223741069/job/16907463318?pr=795

Copy link
Member

@kenjis kenjis left a comment

Choose a reason for hiding this comment

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

LGTM!

@kenjis kenjis merged commit a3030f9 into codeigniter4:develop Sep 19, 2023
25 of 29 checks passed
@kenjis
Copy link
Member

kenjis commented Sep 19, 2023

@tswagger Thank you!

@kenjis
Copy link
Member

kenjis commented Sep 19, 2023

Oh, my dependencies were old.

  - Upgrading codeigniter/coding-standard (v1.7.8 => v1.7.9)
  - Upgrading codeigniter/phpstan-codeigniter (v1.2.0.70400 => v1.3.0.70400)
  - Upgrading nexusphp/cs-config (v3.15.0 => v3.16.0)
  - Upgrading phpstan/phpdoc-parser (1.24.0 => 1.24.1)

Fixed by #840

@kenjis kenjis mentioned this pull request Sep 19, 2023
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature PRs for new features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants