Skip to content
This repository has been archived by the owner on Sep 5, 2022. It is now read-only.

Change aes key to be generated from user provided passcode #27

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jbagnara
Copy link
Contributor

No description provided.

@jbagnara jbagnara requested review from ZPBerg and stephend017 August 14, 2020 23:02
@jbagnara jbagnara self-assigned this Aug 14, 2020
'''
This class handles encryption to prevent identifiable information (facial data)
from leaving the camera. It also generates a random key that will be used by
authorized personnel to acces the data.
'''
self.salt = os.urandom(16) #Salt variable (Generates a random byte string)
self.key = PBKDF2("passphrase", self.salt).read(16) #Creates key using KDF scheme
self.salt = "Embedded2"
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need to save this as a variable? its only used in the constructor

@@ -3,11 +3,11 @@
from typing import List, Tuple

class Encryptor(object):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this class is backwards. Why do we need to re-encapsulate AESEncryptor with Encryptor? I think it would be much better for AESEncryptor to make all encryptors just implement encryptFace and encryptFrame themselves. This will then resolve the weird import statement from src.jetson.AES import Encryption as AESEncryptor which is very misleading.

Copy link
Contributor

@stephend017 stephend017 left a comment

Choose a reason for hiding this comment

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

So after reviewing this we have a few design flaws. essentially the problem is we are writing the generic class Encryptor at the outer level when we should be writing it at the inner level.

@jbagnara
Copy link
Contributor Author

jbagnara commented Sep 6, 2020

@stephend017 Hey Stephen, sorry to address this so late. With school starting and everybody moving to a new team, working on this project has become lower priority and I haven't had time to remove the AES wrapper I've implemented. Would it be alright to merge as-is and address this when the project is picked back up again?

@stephend017
Copy link
Contributor

I don't think we should merge it if the project isn't being actively worked on right now. since it's not a critical bug fix there's really no reason to create a bigger mess for whoever picks it up in the future. They'll have this PR and all of its comments as a starting point

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants