Read/Write Encrypted XML Files #3125

amin1377 · 2025-06-09T16:36:03Z

In this PR, libraries related to encrypting and decrypting files from OpenFPGA have been added. The PugiXML utility has been updated to support reading and writing encrypted files.

soheilshahrouz

There is duplicated header file that should be added as a submodule. Are libdecrypt and libencrypt part of OpenFPGA or they are external libraries?

Which xml files are expected to be encrypted? Is reading encrypted xml files an essential feature of OpenFPGA?

soheilshahrouz · 2025-06-09T21:47:22Z

libs/libdecrypt/src/obfuscate.h

@@ -0,0 +1,182 @@
+#pragma once


This file seems to have come from this repo. I think it should be added as a submodule

soheilshahrouz · 2025-06-09T21:48:48Z

libs/libencrypt/src/obfuscate.h

@@ -0,0 +1,182 @@
+#pragma once


This is exactly the same as another file with the same name. I think we should remove both files and add them as submodules.

amin1377 · 2025-06-09T23:10:02Z

@soheilshahrouz:

I'm not sure it's an "essential" part of OpenFPGA, but it is certainly a useful feature, and we do use it, mainly to read the architecture file.

Within OpenFPGA, it's used directly in the codebase (not as a submodule or external dependency). Given that, I'm inclined to follow the same approach and avoid adding it as a submodule.

AlexandreSinger

Hi @amin1377 ,

I have some concerns with bringing these libraries in. What is the context of this library? Was this copied from somewhere else? I find it a bit strange that this is separated into two separate libraries. I also am surprised a library like this does not already exist. I think rolling our own encryption / decryption flow can come with unwanted baggage.

Also, I think that this should be an optional feature (similar to CapNProto where it will only turn on if CapNProto is installed). I think we should avoid adding dependencies unless it is a feature that we know everyone wants.

.github/scripts/hostsetup.sh

CMakeLists.txt

install_apt_packages.sh

AlexandreSinger · 2025-06-10T02:03:16Z

libs/libdecrypt/ README.md

@@ -0,0 +1,98 @@
+ # libdecrypt


Is this a fully custom library, or something from the internet? I feel like encrypting XML files should not be complicated enough to warent its own library? Are we sure a library for this does not already exist?

I agree with you and have noted both your and Soheil’s concerns. If you’re okay with it, I suggest we prioritize completing the full integration with OpenFPGA first, and we can revisit and roll back any changes we don’t want afterward.

I am not sure I agree. I thought I understood from the meeting last Friday that the goal was to bring in the necessary data structures first, then the necessary features, then the bonus features last. This feels more like a bonus feature which requires a bit more discussion.

My worry about this is that this is another thing that we will need to support in VTR; and encryption can come with a good amount of baggage. We need to really think about how much we want this feature.

AlexandreSinger · 2025-06-10T02:05:47Z

libs/libdecrypt/CMakeLists.txt

+add_compile_definitions(PASS_PHRASE="${PASS_PHRASE}")
+add_compile_definitions(PRIVATE_KEY="${PRIVATE_KEY}")
+find_package(PkgConfig REQUIRED)
+pkg_search_module(OPENSSL REQUIRED openssl)


This line makes OpenSSL a required library for all of VTR. Be aware. I would recommend creating a top-level CMake variable to allow the user to build with this feature or not. I think this is a large dependency for this feature.

I believe that now, with the addition of the new variable VTR_ENABLE_ENCRYPTION to indicate whether the user wants encryption, this CMake will only be triggered if the user wants.

AlexandreSinger · 2025-06-10T02:07:23Z

libs/libdecrypt/config.txt

@@ -0,0 +1 @@
+abcd


What is this file?

According to the README, this file is used if the user chooses to use a passphrase. I added the following line from the README to this file:

Write your passphrase in the file

AlexandreSinger · 2025-06-10T02:07:52Z

libs/libdecrypt/private_key.pem

@@ -0,0 +1,30 @@
+-----BEGIN ENCRYPTED PRIVATE KEY-----


Is it common to include a private key in a repository like this? Shouldnt it be public?

It’s just a placeholder for the user to insert the private key they intend to use.

AlexandreSinger · 2025-06-10T02:11:16Z

libs/libpugiutil/src/pugixml_loc.hpp

@@ -6,7 +6,8 @@

 #include <vector>
 #include "pugixml.hpp"
-
+#include "decryption.h"


Why add the decryption header file to the hpp file and not the CPP file?

AlexandreSinger · 2025-06-10T02:12:10Z

libs/libpugiutil/src/pugixml_util.cpp

+        //store the characters after the '.' from the file_name string
+        result = filename.substr(position);
+    }
+    if (result == ".xmle") {


This should really be an optional feature in my opinion. I think IFDEFing this out would be a good idea.

…rilog-to-routing into read_enc_arch

amin1377 · 2025-06-10T15:27:02Z

Thanks, @AlexandreSinger. What you mentioned about not making it mandatory and instead checking for the requirement in CMake makes sense; I’ll implement that.

As for the way the library is currently used, as I mentioned to Soheil, I’m following the approach used in OpenFPGA since I’m integrating their code. We can revisit and discuss potential changes, such as Soheil’s suggestion to add the repository as a submodule, which I agree with, once the OpenFPGA merging process is complete.

…r to build the encryption/decryption libs

AlexandreSinger · 2025-06-10T19:28:09Z

libs/libpugiutil/src/pugixml_util.cpp

@@ -8,8 +8,37 @@ namespace pugiutil {
 //Returns loc_data look-up for xml node line numbers
 loc_data load_xml(pugi::xml_document& doc,      //Document object to be loaded with file contents
                  const std::string filename) { //Filename to load from
+#ifdef VTR_ENABLE_ENCRYPTION


You are missing something in your CMake file. CPP code cannot read variables from CMake like this. In CMake you are going to need to set this as a define for the PUGI target. See how this is done for Eigen in the AP flow.

AlexandreSinger · 2025-06-10T19:28:38Z

libs/libpugiutil/src/pugixml_util.cpp

@@ -8,8 +8,37 @@ namespace pugiutil {
 //Returns loc_data look-up for xml node line numbers
 loc_data load_xml(pugi::xml_document& doc,      //Document object to be loaded with file contents
                  const std::string filename) { //Filename to load from
+#ifdef VTR_ENABLE_ENCRYPTION
+#include "decryption.h"


Normally to do this we still put the header file at the top of the file and wrap ifdefs around it. Its a bit strange to put the header all the way down here.

AlexandreSinger · 2025-06-10T19:28:52Z

libs/libpugiutil/src/pugixml_util.cpp

+        delete[] final;
+        return location_data;
+    }
+#endif


Minor nit: I would comment what this is ending.

AlexandreSinger · 2025-06-10T19:29:13Z

libs/libpugiutil/src/pugixml_util.cpp

+        return location_data;
+    }
+#endif
+    //auto location_data = loc_data(end_result_fname);


Commented code.

amin1377 · 2025-06-10T20:10:37Z

After discussing with @AlexandreSinger and @vaughnbetz, we decided to close this PR.

amin1377 added 3 commits June 9, 2025 12:01

[script] add ssl lib

23f425b

[libs][enc] add encrypt/decrypt libs

95e3a39

[libs][pugiutil] read from an encrypted file

62ba644

github-actions bot added lang-cpp C/C++ code infra Project Infrastructure build Build system scripts Utility & Infrastructure scripts lang-make CMake/Make code lang-shell Shell scripts (bash etc.) libpugiutil labels Jun 9, 2025

amin1377 added 2 commits June 9, 2025 12:40

[libs][enc] use prama once

efb6bb3

make format

954ce61

amin1377 requested review from soheilshahrouz, AmirhosseinPoolad and AlexandreSinger June 9, 2025 16:44

amin1377 added 2 commits June 9, 2025 15:31

[lib][d/encrypt] use unique namesapce

351026a

[libs][encrypt] replace all AY instances with enc/dec

3062b40

soheilshahrouz requested changes Jun 9, 2025

View reviewed changes

AlexandreSinger reviewed Jun 10, 2025

View reviewed changes

amin1377 added 2 commits June 10, 2025 09:40

Merge branch 'master' of https://github.com/verilog-to-routing/vtr-ve…

b9096af

…rilog-to-routing into read_enc_arch

[libs][arch] add xmle as an acceptable architecture format

42e0b0e

github-actions bot added the libarchfpga Library for handling FPGA Architecture descriptions label Jun 10, 2025

amin1377 added 2 commits June 10, 2025 12:16

[libs][encryption] add VTR_ENABLE_ENCRYPTION flag to determine whethe…

10018e1

…r to build the encryption/decryption libs

make format

e014ebc

AlexandreSinger reviewed Jun 10, 2025

View reviewed changes

amin1377 closed this Jun 10, 2025

		@@ -0,0 +1 @@
		abcd

Read/Write Encrypted XML Files #3125

Read/Write Encrypted XML Files #3125

Uh oh!

Conversation

amin1377 commented Jun 9, 2025

Uh oh!

soheilshahrouz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soheilshahrouz Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amin1377 commented Jun 9, 2025

Uh oh!

AlexandreSinger left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amin1377 commented Jun 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amin1377 commented Jun 10, 2025

Uh oh!

Uh oh!

soheilshahrouz Jun 9, 2025 •

edited

Loading