JRCSTU / co2wui

WebUI for co2mpas
https://co2mpas.io
European Union Public License 1.1
0 stars 0 forks source link

Load keys #76

Closed sapofra closed 5 years ago

sapofra commented 5 years ago

image (As wrote the 30th of May), notice the part in yellow.

Addition form #125: co2mpas_gui should not require sign.co2mpas.key. If the sign.co2mpas.key is not in the DICE_KEYS folder, it will be automatically generated.

stefanocorsi commented 5 years ago

We have decided (meeting 4/9) to bin the idea of uploading the keys and instead open a windows explorer window in the directory where the keys are. @vinci1it2000 you mentioned a way with Flask to do this, could you post it here? I can't figure out how, because I see that Firefox and Exporer accept a file:// link, but Chrome won't.

ankostis commented 5 years ago

That would require extra work with dubious results.

Browsers for security reasons are forbidden from accessing properly the local filesystem, which means that it is the Flask-server that has to do it (Implementing a file-explorer ala electron in the co2wui is out of the question, i guess).

Now flask, being a pure python, cannot simply open file-explorer; it has to use the Tk gui library. That's the library of the old co2mpas GUI. So are we proposing to go back to the old situation for some part of the project?

From a UX standpoint, it would be uncanny for a web app to "block", while an explorer window is launched in the background.


The safest approach is to have a textbox on the WUI where the use can set the path location of the keyfiles. She can copy the path from explorer. Low-tech, but it works.

vinci1it2000 commented 5 years ago
@app.route('/open_home_folder', methods=['GET'])
def open_home_folder():
    import os
    if os.environ.get('DISABLE_HOME_FOLDER')  != 'True':
        import webbrowser
        webbrowser.open('file:///' + os.path.abspath(HOME_FOLDER))
        return  'The co2mpas home folder is being open by the system's file manager.'
ankostis commented 5 years ago

That won't do it. That's another browser window loading content from local filesystem.

A window can load either from the filesystem, or from the internet (in our case that is the Flask server). And these two cannot mix for security reasons: https://security.stackexchange.com/questions/201208/why-do-browsers-disallow-accessing-files-from-local-file-system-even-if-the-html

vinci1it2000 commented 5 years ago

The command webbrowser.open('file:///' + os.path.abspath(HOME_FOLDER)) opens the system's file manager (i.e., File Explorer in windows, Finder in mac, etc). The scope of this button is not to load the keys, but just to show/open the home directory of co2mpas. We decided to remove the load keys page, because it is just for TA and we don't want to show and explain to normal user this functionality (that is explained on DICE for the authorised users). Consider that this button in the GUI is enabled for the desktop application and should be disabled in case of a web application.

ankostis commented 5 years ago

Then what you would need is os.startfile('path/to/keys'), not webbrowser.open('file:\\path\to\keys').
Actually, webbrowser.open() delegates internally to os.openfile() on Windows. In any case, this hack has a serious drawback if the flask-server runs on another PC: the file-explorer window will open in that PC, not on the client's machine.

I still cannot understand how the file-explorer window would help the app? I mean, is the new window just for informing the user where the keys files are located, or to "select" them? Because the later is forbidden due to security reasons, you cannot pass the selected folder path back into the flask-app.

Am i missing something?

vinci1it2000 commented 5 years ago

The os.startfile is available just for windows, while webbrowser.open is cross-platform. The idea is to inform the user where he/she can find the home folder of co2mpas. This folder contains:

The environment variable DISABLE_HOME_FOLDER is used to avoid the opening of a folder if the back-end runs remotely.

ankostis commented 5 years ago

Aha, it's only for informational purposes - the keys-folder cannot change.

Then there is a very simple solution, just add this link somewhere on the co2wui page:

<a href="file:///absolute-path/to/key/files" target="_blank">co2mpas keys folder</a>

No server API needed, no roundtrip to python code, simple html that works on all platforms regardless of the default browser. Most importantly, there is no danger if you forget to set the environment variable - if it runs on a different PC it would simply open the client PC's folder, which is ok, misleading, but it won't break the server.

Btw you can show the link conditionally, only when the host-part of the url is the localhost, and get away with the env-var altogether.

stefanocorsi commented 5 years ago

At the moment this is the way the GUI behaves, trying to have the best of the two worlds:

1) It has a Key management menu item loading to a form that allows the user to select and delete keys in a friendly manner

immagine

2) The option for the secret password file has been removed

3) It has a link to open the key folder

immagine

4) Keys are saved in a "keys" folder under the .co2mpas folder in the user's home directory. I suppose (@vinci1it2000) that I should rename the folder DICE_KEYS?

5) It's not possible now to run TA mode without encryption key and signature key defined.

ankostis commented 5 years ago

I believe we are fine here. @sapofra @vinci1it2000 are you ok?

vinci1it2000 commented 5 years ago

With @stefanocorsi we have decided to remove completely this page and simply add a button somewhere that open the home folder of co2mpas. @stefanocorsi regarding the renaming.. yes you should.

gfon commented 5 years ago

May I ask why you remove this page? So there was a solution proposed 4 days ago by Stefano, and now you removed it? Seen from a user perspective it looked helpful.

stefanocorsi commented 5 years ago

@vinci1it2000 could you please explain the arguments you presented to me regarding a possible confusion for users?

vinci1it2000 commented 5 years ago

We decided to remove the page because it creates confusion. To current TA users, it will look like a new feature, so we will receive questions about this new page. For non-TA users, it is a feature that does not concern them, and since they are not aware of dice key they might send us questions. Practically, by removing the page we avoid receiving several questions. Moreover, we don't want to change the procedure that has been described in the DICE3 manual. In addition, showing directly the DICE_KEYS folder we are more transparent and they are aware of the location of the keys.

dimitriskomnos commented 5 years ago

for the moment the keys, and everything (input files, output files) for the wui are stored inside a path not easily accessible from the user (C:/Users/username/.co2wui). It is not the path that the user installs something (e.g C:/Apps).

So in this case, where should this folder DICE_KEYS created? How will the co2wui trace this path?

stefanocorsi commented 5 years ago

The best way to go is that we all work with the .co2mpas dir in the home directory for the user.

We have this function in the co2wui source:

def _home_fpath() -> Path:

    if "CO2MPAS_HOME" in os.environ:
        home = Path(os.environ["CO2MPAS_HOME"])
    else:
        home = Path.home() / ".co2mpas"
    return home

I propose that we all follow the same convention.

In the .co2mpas dir we have the following dirs/subdirs:

        ("DICE_KEYS",),
        ("input",),
        ("output",),
        ("sync", "input"),
        ("sync", "output"),
gfon commented 5 years ago

@vinci1it2000 Who decided? you say we I understand from the reactions above that it was basically you who decided.

@stefanocorsi @ankostis @spycon69

Do you agree with the arguments and the approach above?

Is this inline with the webased architecture we agreed earlier on?

I sense there is no consensus here. In any case I don't want to open new issues 5 days before code freeze, but I don't consider this issue closed unless I receive consesus from all of you.

Thanks Giorgos

stefanocorsi commented 5 years ago

Ok, so in order to close on this issue we need:

@vinci1it2000 to sync with me on the message above (use of the same convention for the .co2mpas dirs).

@gfon and @spycon69 to decide on the presence of the user friendly key management page.

stefanocorsi commented 5 years ago

@gfon I consider it natural for the app to have a visual management of the keys, unless there are strong indications against it. It's important as well that the whole of co2mpas (both command line and GUI) work on the same directories.

gfon commented 5 years ago

@stefanocorsi thanks for the feedback, look we are a team, so I would like to hear also the thoughts of @ankostis, @spycon69, @haphyp, or the others involved in the CO2MPAS/DICE development, if they have any in addition to yours and @vinci1it2000 .

For me priority 1st is security above everything else, second is software stability and robustness, then fast and effective operations for us and the user, and then user friendliness. Of course we need to think also about the future where do we want to go, eg in case co2mpas/dice is used in the future for receiving real world, or in service data.

I believe if in the end you discuss (not endlessly) with arguments, you can find consensus or a majority.

If you don't find consensus @spycon69 please draft a short list of bullet points with the pros and cons of both options and I will decide on Friday when back from BRU. But I 'd prefer not reach this point and surely don't feel comfortable to take such decisions in airports.

spycon69 commented 5 years ago

Indifferent from experienced or new users and having in mind that the CO2MPAS platform has a web-based future I strongly believe that it is more user-friendly to keep the Key Management page.

vinci1it2000 commented 5 years ago

The removal of this feature has been decided in a meeting with @stefanocorsi, @sapofra, @spycon69, @dimitriskomnos, and @vinci1it2000. It was not my decision, but a proposal that has been approved. If now we are going in another direction, it is fine. Should we do another meeting?

vinci1it2000 commented 5 years ago

@stefanocorsi regarding the folder structure it is fine.

ankostis commented 5 years ago

It is also important the the co2wui can function as a proper remote multi-user web-server,
think of ...

Local-path hacks don't fit into this model, so i say that we can live with the above decision for now , but it has to be reverted as soon as this release is made.

spycon69 commented 5 years ago

Almost in every project there are change requests even at the latest stages of development. Provided that are justified we need to be agile and accommodate them. In that sense, if there is enough time I propose to implement the change. Otherwise I would agree with @ankostis to implement it right after the release is made.

spycon69 commented 5 years ago

@stefanocorsi As already mentioned above we must keep in mind that the future evolution of Co2mpas would be web-based. Therefore my proposal is to keep the page. Besides that at this stage we need to focus on testing and validation since time is pressing.

spycon69 commented 5 years ago

@ankostis I would like your input in regards to any potential security threats IF we decide to leave the "Load Keys" page as it is.

ankostis commented 5 years ago

There are various reasons why all browsers stopped allowing opening local-links from regular pages. And local-link are a no-go if we wish co2mpas to work from remote, as a regular web client-server application, for which Stefano's code is the typical way.

DICE instructions should be enough for users to manage their keys for now - so we can and should drop the local folder.

Should Stefano's code be re-enabled? i'm ok with both ways (IN or OUT). True, it is not strictly needed right now, since

Vinz concerns about the new functionality clashing with the DICE instructions are also valid, afterall, he wrote those instructions, but i wouldn't consider that "cast in stone" - we can amend the instructions.

The key factor is whether we have enough time to test the feature and amend the instructions, given the code-freeze deadline?

ankostis commented 5 years ago

Another important thing.

I feel awkward to consent on a revision of a prior decision of the team. My apologies, i tried to react as quick as possible when i was informed about that decision that would compromise the security of the app, and "train" our users on the wrong paradigm.

spycon69 commented 5 years ago

@vinci1it2000 can we have the estimate effort for amending the instructions ?

spycon69 commented 5 years ago

I was referring to DICE instructions

stefanocorsi commented 5 years ago

So, considering the flow of the discussion, please let me lmow if this could be a satisfying outcome:

spycon69 commented 5 years ago

Agreed on both. Since I do not want at this point to miss the Sep 27th code-freeze deadline I would suggest to remove the keys page and finalize the development.

ankostis commented 5 years ago

... and barely open the folder

Remove also the local link.

stefanocorsi commented 5 years ago

Ok @ankostis I will remove the Key management link and corresponding action.

stefanocorsi commented 5 years ago

I'm very proud to have committed the removal of the key management and to close this venerable issue! :-)

vinci1it2000 commented 5 years ago

@ankostis @stefanocorsi @spycon69 @gfon . Just for the future in case we want to go for a web based co2mpas (i.e., a centralized JRC server), the idea of having this page and this functionality (i.e., data encryption) is not feasible because it is vulnerable and irrational. The reason is because the user needs to encrypt the sensible data before sending the data. Hence, this functionality have to be enabled on a local machine and not on the server.

If you want to build it, I can easily show the vulnerability/irrationality describing the logical workflow of an hypothetical co2mpas website:

1) the user uploads the sensible data unencrypted (a simple man in the middle can steal all data), 2) the sensible data are stored on the server without any security (everyone that has access to the server or to the HD can steal the sensible data) 3) the server stores the user’s private signature key that have to be uploaded somehow (so the key can be stolen using the same methods described above) 4) finally, the server encrypts the sensible data so the user can download the co2mpas.ta file and submit it again on the same server... mmmm... I think that at this point is illogic... the server has the sensible data unencrypted and encrypted!

@ankostis you don’t want to be in the list of NIST, right? Because if we do something like this, I think that we can have a great chance to be included.

If someone has any idea to overcome all these security problems, he/she is more than welcome.

ankostis commented 5 years ago

Bullseye. Key-functions do not belong to the co2wui server.

The idea to overcome all this is to use client-side cryptography using javascript libraries, e.g. PKI.js, but that means we replace co2mpas_dice project. Stefano's WUI html code will be useful, if we ever decide to go down this road.